-
Notifications
You must be signed in to change notification settings - Fork 633
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
Conditionally create server spans for falcon #867
Merged
ocelotl
merged 8 commits into
open-telemetry:main
from
ashu658:conditional-server-span-falcon
Jan 21, 2022
Merged
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
1a44fb7
Making span as internal for falcon in presence of a span in current c…
c12c54c
Updating changelog
721b15a
Fixing lint and generate build failures
9c7847f
Resolving comments: Converting snippet to re-usable function
cae0057
Fixing build failures
b9b8efb
Resolving comments: Creating wrapper for start span to make internal/…
85659a6
Rerun docker tests
95b76bf
Resolving comments: Refactoring
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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 was thinking more like a wrapper function for
start_span
which internally takes care of checking server spans something likethis function would accept whatever is necessary to figure out if an existing span is present or not and decide internally whether to create a server span or an internal span, and which context to use as parent.
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.
This function will need to return token also along with span.
I am not sure if this is okay.
thinking of putting this function in
instrumentation/utils.py
Also, we can shorten the recurring snippet if we want to skip the function
as the else part in the snippet is not necessary. Something like this:
Please share your thoughts.
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.
That sounds OK. It can return the span + token and let the caller store a reference to the token and detach it at a later time.
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.
The main issue is that this is a very specific piece of logic strictly followed by all server instrumentation so I think it makes sense to encapsulate the logic in a re-usable function that is easily discoverable and changeable in a single place. Number of lines of code is not a big deal.
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.
Makes sense
Created wrapper for start_span.