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
feat: introduces ability to suppress tracing via context #1344
feat: introduces ability to suppress tracing via context #1344
Changes from 11 commits
207dfdc
f5d351c
98acb74
c2ac8b4
4d7e5ea
9a351ed
df7e85d
d588d4f
91d1add
9eb2d5e
d183bd8
4ff9591
6bb2eb6
49f53e4
74a4000
2866f51
6c81a98
cc0249b
f125085
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.
I would remove the extraneous cast and assignment, but this is minor.
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 definitely giving that the side-eye while I was restructuring. Thanks for the extra push. :)
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 probably minor, but since this function is likely to be called a lot, I think the performance benefit alone of skipping the assignment is worth it since you don't really lose anything here.
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.
please add some doc
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.
@obecny what sort of doc are you looking for?
do you want a comment on why this is protected? that seems like something that would quickly get out of date with future usages, outside of the obvious fact it is now available to classes extending the InMemorySpanExporter.
or are you looking for something else?
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 something is protected its meaning should be documented so that when someone subclasses this they know what the property represents. It could be argued it is obvious in this case, but it is good practice in general.
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.
OK. i don't see that done for any other protected members (non-function) in the code base and somewhat debate the value here but am happy to go along with the standards y'all are looking for.
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.
@michaelgoin I agree we haven't been great at being consistent about this, and in many cases the value is questionable anyways.