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
A72: OpenTelemetry Tracing #389
A72: OpenTelemetry Tracing #389
Changes from 2 commits
1d5b48c
67e34e8
392a533
7f668a7
0041641
7a6e596
6bc027e
5bb3823
5dbbc13
7b48df4
8631cee
528dd82
cf24140
517f340
8a0f8f7
942b9b6
eea0571
a2c420e
a39af0d
50565d6
28d9f51
2d89763
032beee
0275820
3e26373
3885cd8
c866de5
a346c24
66b40d4
41c2ed2
68443a0
a199618
8afa317
78f3d71
9a219d3
06ec130
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 think we talked about this earlier, but "sent" in "Outbound message sent" is confusing. Can we drop it?
For C++, this would otherwise mean that we would first see -
"Outbound message sent" and then "Outbound message compressed"
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 looks fine. "sent" means this trace happens somewhere from application to the wire, and we made that vague and general(decision made from a discussion). Are you confused because compression should not happen after "sent"?
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.
how was the namespacing for "message.message.id" decided?
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 was from the Otel java opencensus shim. Languages can be different.
I think whenever possible, we can make the name generated from the shim and the one generated directly from otel be consistent. This is not happening because the naming in our census needs improvements. And I am fine with that. @markdroth @ejona86
(https://github.com/open-telemetry/opentelemetry-java/blob/main/opencensus-shim/src/main/java/io/opentelemetry/opencensusshim/SpanConverter.java#L24-L27)
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 don't think we have to copy the shim's behavior.
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 proposed new names. Let's move forward and fix/vote in API stabilization.