Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NH-26617 add Python framework versions to init msg #94

Merged
merged 11 commits into from
Jan 5, 2023

Conversation

tammy-baylis-swi
Copy link
Contributor

@tammy-baylis-swi tammy-baylis-swi commented Dec 16, 2022

Here is one way to add instrumented Python framework versions to the init message. It duplicates some parts of autoinstrumentation's _load_instrumentors method by checking available instrumentation entry points, skipping if in OTEL_PYTHON_DISABLED_INSTRUMENTATIONS or if conflict or if not importable/checkable. If we can check a framework's __version__ or equivalent (special cases added in a1fc5ee and more), then it is added to the __Init message as Python.*.Version

Regarding special cases: Some of Python packages don't have __version__ ☹️ and there's probably more than just urllib and sqlite3 so there are several elifs here.

The approach is different than using InstrumentScope of finished spans (#92) because that can only hold one instrumentation library per span and there is no meaningful completed span at time of distro configuration. See also doc for "APM Python InstrumentationScope".

EDIT: Some other approaches I thought about but didn't do: (A) Maintain a version-controlled map of all opentelemetry.instrumention.* libraries and their corresponding Python frameworks, with expressions of how to check framework version (seems very manual but could cut down on cases in code). (B) Override BaseDistro.load_instrumentor to check Python framework versions as instrumentation libraries are loaded, then store that info for the Configurator to use at __Init message. I couldn't figure out if there is a smart way to share info between Distro and Configurator (see also how they're loaded in doc for startup).

Example __Init message from test collector (EDIT: updated 2023-01-04 x2):

{
    "_V": "1",
    "sw.trace_context": "00-40407385fb180d076092ee6b1c41ccdf-0c0afed155292d1c-01",
    "X-Trace": "2B40407385FB180D076092EE6B1C41CCDF000000000C0AFED155292D1C01",
    "sw.parent_span_id": "2ea6d6c124b75f83",
    "Edge": [
        "2EA6D6C124B75F83"
    ],
    "Layer": "Python",
    "__Init": true,
    "telemetry.sdk.language": "python",
    "telemetry.sdk.name": "opentelemetry",
    "telemetry.sdk.version": "1.15.0",
    "process.runtime.version": "3.9.16",
    "process.runtime.name": "cpython",
    "process.runtime.description": "3.9.16 (main, Dec 21 2022, 19:18:44) \n[GCC 10.2.1 20210110]",
    "process.executable.path": "/usr/local/bin/python",
    "APM.Version": "0.4.0",
    "APM.Extension.Version": "11.1.0",
    "Python.falcon.Version": "3.1.1",
    "Python.asyncpg.Version": "0.27.0",
    "Python.jinja2.Version": "3.1.2",
    "Python.redis.Version": "4.4.0",
    "Python.botocore.Version": "1.29.43",
    "Python.mysql.Version": "8.0.29",
    "Python.pymongo.Version": "4.3.3",
    "Python.urllib.Version": "3.9",
    "Python.system_metrics.Version": "5.9.4",
    "Python.requests.Version": "2.28.1",
    "Python.flask.Version": "2.2.2",
    "Python.sqlalchemy.Version": "1.4.46",
    "Python.boto.Version": "2.49.0",
    "Python.elasticsearch.Version": "7.17.0",
    "Python.django.Version": "4.1.5",
    "Python.tornado.Version": "6.2",
    "Python.fastapi.Version": "0.88.0",
    "Python.pymysql.Version": "1.0.2",
    "Python.psycopg2.Version": "2.9.5 (dt dec pq3 ext lo64)",
    "Python.kafka.Version": "2.0.2",
    "Python.sqlite3.Version": "3.34.1",
    "Python.confluent_kafka.Version": "1.9.2",
    "Python.grpc_aio_client.Version": "1.51.1",
    "Python.grpc_aio_server.Version": "1.51.1",
    "Python.grpc_client.Version": "1.51.1",
    "Python.grpc_server.Version": "1.51.1",
    "Python.pika.Version": "1.3.1",
    "Python.tortoiseorm.Version": "0.19.2",
    "Python.logging.Version": "0.5.1.2",
    "Python.pyramid.Version": "2.0",
    "Python.aiohttp-client.Version": "3.8.3",
    "Python.remoulade.Version": "0.54.2",
    "Python.celery.Version": "5.2.7",
    "Python.boto3.Version": "1.26.43",
    "Timestamp_u": 1672876626017494,
    "TID": 1,
    "Hostname": "de6a05f7288a"
}

Please let me know if any questions or suggestions!

@tammy-baylis-swi tammy-baylis-swi requested review from cheempz and a team December 16, 2022 00:52
@tammy-baylis-swi tammy-baylis-swi changed the title NhH-26617 add Python framework versions to init msg NH-26617 add Python framework versions to init msg Dec 16, 2022
Copy link
Member

@raphael-theriault-swi raphael-theriault-swi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks all good !

@tammy-baylis-swi
Copy link
Contributor Author

This PR isn't quite ready for a re-review. I've added more cases for more Python libraries with updated testbed. Now, sometimes and not always ☹️ , liboboe is erroring at the last addInfo loop in the _report_init_event method (TypeError: Wrong number or type of arguments for overloaded function 'Event_addInfo'.). Going to fix this!

cheempz
cheempz previously approved these changes Dec 29, 2022
Copy link
Contributor

@cheempz cheempz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

Base automatically changed from NH-26617-std-init-message to main January 3, 2023 23:27
@tammy-baylis-swi
Copy link
Contributor Author

Thanks @cheempz and @raphael-theriault-swi for review! Please could you have another look at this? Some changes:

  1. Merged main in so no more AppOptics KVs included -- see example in updated PR description.
  2. The testbed (at this PR) doesn't error for me anymore, I think because of 287a8fb.
  3. Also in the example in updated PR description are several more KVs for different Python frameworks than first submission, now that there are several more elifs. Do the KVs look ok and informative enough? For example I'm looking at Python.Grpc_aio_client.Version and thinking maybe it should be Python.Grpc_Aio_Client.Version instead.

@raphael-theriault-swi
Copy link
Member

Yeah I think it's worth either keeping the KVs untouched or changing the case to be consistent, in my opinion at least both Python.grpc_aio_client.Version and Python.Grpc_Aio_Client.Version are more readable at a glance than Python.Grpc_aio_client.Version

@cheempz
Copy link
Contributor

cheempz commented Jan 4, 2023

Yeah I think it's worth either keeping the KVs untouched or changing the case to be consistent, in my opinion at least both Python.grpc_aio_client.Version and Python.Grpc_Aio_Client.Version are more readable at a glance than Python.Grpc_aio_client.Version

Agree, slight preference for keeping untouched -- thanks for spotting this @tammy-baylis-swi.

@tammy-baylis-swi
Copy link
Contributor Author

Thanks again! Switched to Python.<component>.Version KVs where <component> is untouched in bffb2c9.

Copy link
Member

@raphael-theriault-swi raphael-theriault-swi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me !

@tammy-baylis-swi tammy-baylis-swi merged commit d2aec4b into main Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants