Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add instrumentation for threading module #1582
Add instrumentation for threading module #1582
Changes from 28 commits
12e3fff
3d1ec3a
bcfe04a
46c5394
5456f9b
fb8df3c
ca8b114
c8d8f69
dcd388b
9e3b007
d3a2d90
c13a4b7
4d5625f
260c7f9
aec3ff9
5cc3e6f
bf2b5bb
3704f68
4c100ea
2a38a50
5e6bd84
c528784
075107c
2f89aaa
2b22b2f
147f3d4
a70ea70
971e864
8f50ce4
9315652
2181ccc
22a396f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It doesn't look we added this page to the docs. Need an RST in /~https://github.com/open-telemetry/opentelemetry-python-contrib/tree/main/docs/instrumentation right?
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.
Will add an RST to the docs.
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.
If you add a docs page, it should pull in the module docstring with autodoc.
Afaict this instrumentation only propagates context to child threads. Can you add a docstring explaining what this does and any caveats?
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.
Yes
I think this is a good point and we should document the fact that this initial version doesn't support user subclassed threading.
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.
Where should I add this docstring?
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.
Here is an example
opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py
Lines 15 to 61 in 4199751
We should also add that this enables the context propagation for threaded programs and that it doesn't currently support the subclassed user threads.
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.
Is this method necessary here? From the docs:
Also:
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.
If you did
self._parent_span = get_current_span()
in__init__()
, the new thread would inherit the context of the thread that created the new thread object. Here, the new thread inherits the context of the thread that callsstart()
.In the vast majority of cases they are probably the same caller though. Is this an intentional distinction @pridhi-arora ? Otherwise I agree with Diego that would be simpler
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.
But the current context may be different when one creates the thread object vs when it gets called. Take the following scenario.
It would be incorrect to use any span context other than "foo" right? We need this for that reason.
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.
From the python docs
This looks good for the first way (passing a callable) but it won't work right for second way of subclassing Thread, correct?
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.
Unless they call
super().run()
in the overridenrun
.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.
Yes but users shouldn't have to modify their code for this instrumentation to work
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.
Ah, I see, they may not already have that call in their code.
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.
It's OK if we leave this out of scope for now, we should just document the caveat
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.
In that case, this may work:
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.
I would suggest to leave this out of scope for the initial version and address it separately.