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
use BoundedAttributes for attributes in link, event, resource, spans #1915
use BoundedAttributes for attributes in link, event, resource, spans #1915
Changes from 6 commits
e7550e7
f237bae
a8e923a
c00d04a
09ca0a3
9bd9596
736500d
c9662ed
943b078
f05094d
f29c775
48a378d
8a84a0a
1b98aa9
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.
attributes
makes sense in terms of what we are usingBoundedDict
for, but for the class itself it doesn't make much sense. Should we have a different name instead ofBoundedDict
?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 would be good to make a separation from the SDK
BoundedDict
too.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.
Yeah I considered for a minute making
Attributes
a class to replaceBoundedDict
, wasn't sure if this would be doable without breaking Attributes' current interface thoughThere 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.
Maybe just
BoundedAttributes
or something?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.
Done.
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.
self._dict.popitem(last=False)
?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'm curious what people's thoughts on this are. It's confusing to me that if the attributes dict is full, we delete an existing item from the dictionary in favour of the new item. Any thoughts @lzchen?
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.
Yeah my first impression when reading the specs is that the LATEST item that is attempting to be added would be dropped instead of popping the first item that was added. If we decide on changing the functionality, you could leave that for a separate PR if you wish.
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.
What do other SIGs do? And also spec says There SHOULD be a log emitted to indicate to the user that an attribute, event, or link was discarded due to such a limit. To prevent excessive logging, the log should not be emitted once per span, or per discarded attribute, event, or links. https://github.com/open-telemetry/opentelemetry-specification/blob/b46bcab5fb709381f1fd52096a19541370c7d1b3/specification/trace/sdk.md#span-limits
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 like go does the same thing as we do currently. https://github.com/open-telemetry/opentelemetry-go/blob/c1f460e097d395fa7cd02acc4a6096d6c6f14b08/sdk/trace/attributesmap.go#L45-L62
I agree that for now it probably doesn't make sense to change the behaviour
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.
Are these placeholders until this is merged? Would be good to have a TODO.
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.
Captured the work in an issue #1918