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
chore: add Open Telemetry attributes to grpc spans #698
chore: add Open Telemetry attributes to grpc spans #698
Changes from 17 commits
9efc28a
3d1c073
3cdfe80
4f4a936
47e9d82
fda2d1c
420b385
7c0e025
5d13200
89c934e
8c195db
9f3ede7
d891882
2f6c71d
3d4a941
2f52fea
099764e
20ee988
270e3c6
6091af1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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 is the one that the spec says may be the DNS name if no DNS lookup is required.
I'm wondering if we shouldn't use the
text_map_propagator
to cheat the system a bit, and on the client side injectblock-producer
andrpc
as an additional property.We could then use that here, and otherwise default to the IP if its missing.
Could also be done as a follow-up PR - I imagine you might be very much over this back-and-forth :D
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.
Not at all - happy to iterate on this PR as much as required.
I'm a bit dubious as to the utility of that functionality but happy to add that to this PR if you think it is worthwhile.
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.
The main reason I'm considering it is because our components can (and currently do) run on the same server instance (and same process). Which means for the inter-component comms, the client and server IP address will just be the same since they're the same for all three components.
So I think it would help having the "caller" identifiable at least. Though maybe instead we should be hosting different api endpoints for this internal communication in any case e.g.
store.block-producer/get_block_inputs
andstore.rpc/get_state
, which would do the same job and provide some additional decoupling.I'm happy to just punt this to a separate issue instead.
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.
Ah ok yea that makes sense. Thanks for elaborating