-
Notifications
You must be signed in to change notification settings - Fork 1
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
NH-26617 add Python framework versions to init msg #94
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks all good !
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
Thanks @cheempz and @raphael-theriault-swi for review! Please could you have another look at this? Some changes:
|
Yeah I think it's worth either keeping the KVs untouched or changing the case to be consistent, in my opinion at least both |
Agree, slight preference for keeping untouched -- thanks for spotting this @tammy-baylis-swi. |
Thanks again! Switched to |
There was a problem hiding this 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 !
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 asPython.*.Version
Regarding special cases: Some of Python packages don't have☹️
__version__
and there's probably more than just urllib and sqlite3so 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) OverrideBaseDistro.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):
Please let me know if any questions or suggestions!