-
Notifications
You must be signed in to change notification settings - Fork 896
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 note about reporting dropped attributes/events/links #1699
add note about reporting dropped attributes/events/links #1699
Conversation
specification/trace/sdk.md
Outdated
@@ -356,6 +356,10 @@ 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. | |||
|
|||
Counts for attributes, events and links dropped due to collection limits MUST be reported |
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 think this MUST is too much, as it depends on the protocol. What we should say is that the counts MUST be available for exporters. I think this would best fit in the "readable span" wording, somewhere around /~https://github.com/open-telemetry/opentelemetry-specification/pull/1699/files#diff-ace73543d690260e873d774428419c23895283b6c31177d405816b154d1f5aa0R91
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 @Oberon00, I updated the note, PTAL.
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 think it would be best to move this wording to the "readable span" definition, somewhere around line 91, as this will probably be the part where this wording has to be implemented. But semantically, the change looks good to me!
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.
@codeboten Could you update this part according to @Oberon00 ?
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.
LGTM, but see #1699 (comment)
Fixes #1698
Changes
Clarify that dropped counts should be reported from the SDK. Would love feedback from reviewers on whether or not this change should include a change to the matrix.
Related issues #1656