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
Trace SDK: Provide definitions for readable and read/write span. #669
Trace SDK: Provide definitions for readable and read/write span. #669
Changes from all commits
b89b9db
31e8f37
86d26a2
91eafdc
9a11b76
b59b69a
14a5a17
a10b82f
45e2b61
6bc4531
6ea4067
59fb611
c9a5d44
3c3bf22
95dae9b
8a14195
179fcc1
0b9709d
3866bb1
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.
Is this really needed? This may make user to do hacks. Imagine we have this interface in the OnStart, they know that the object we return implements both, OnEnd we return only read, then they will guess is the same interface and may do a cast to the ReadWrite interface.
My suggestion is that Span is the write interface and ReadableSpan is the read interface, if access required to both we should pass both interfaces. Maybe I am to "evil" and think of possible hacks and abuses, but I've seen all of these happening in my previous life :)
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.
You could pass a wrapper object that only implements the readable interface to protect against that.
I think we can mention passing the read-only and write-only interface separately as an allowed implementation option for read/write span though.
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.
@Oberon00 suggested I do that hack when I need to mutate the span in
OnEnd
:) I think there is a need to be able to mutate spans globally before export (usually to use other tags as signals to user-defined domain-specific tags) so hope we can find a way. The issue we were worried about is the ordering betweenOnEnd
and exportThere 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.
More importantly, spans are already ended at that point and can't be modified anyway. #669 (comment)
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.
@bogdandrutu I incorporated the "read/write implemented as two separate objects" option in 0b9709d.