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
otelgrpc: Implement grpc.StatsHandler for trace instrumentation #3002
otelgrpc: Implement grpc.StatsHandler for trace instrumentation #3002
Changes from 8 commits
2a4bde0
7567e0d
92110c0
afebd44
abea6d6
03fcd32
51cff71
b49dac3
1468039
3cd5fd5
356c723
7c9c665
7d24364
550afc5
c22bea0
87dbe38
3258d1d
0213f6b
0b66e2e
1c4fb80
494eb36
256b144
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.
May I suggest to implement options to configure this behavior. Similar to how
net/http
instrumentation works, see:opentelemetry-go-contrib/instrumentation/net/http/otelhttp/config.go
Lines 106 to 124 in 75d0c13
I.e. default to private endpoints (i.e.
parent
relationship between spans/traces) and switch to public (i.e.link
relationship) when explicitly asked to.p.s. I think maintainers should probably decide and document how this should work across all integrations to provide consistent experience to users.
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.
This maintains the behavior of the interceptor grpc implementation:
opentelemetry-go-contrib/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go
Line 342 in 9a582bd
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 does maintain the existing behavior but at the same time now is the chance to introduce new default behavior, consistent with
net/http
instrumentation. If not now, then it'll be a breaking change for anyone who switches to the new implementation. Breaking change may be fine if this implementation is marked as experimental, I don't mind that or addressing it separately, I just need such an option. Thanks!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 will open another issue to tackle this problem after this pr is merged.
Check warning on line 179 in instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go
Codecov / codecov/patch
instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go#L177-L179