-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat(p2p): cover Exchange with traces #150
feat(p2p): cover Exchange with traces #150
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #150 +/- ##
==========================================
- Coverage 63.96% 62.95% -1.02%
==========================================
Files 39 39
Lines 3383 3593 +210
==========================================
+ Hits 2164 2262 +98
- Misses 1052 1157 +105
- Partials 167 174 +7 ☔ View full report in Codecov by Sentry. |
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.
Would be also nice to cover Head.
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.
Just a general comment, there seems to be duplication of traces in paths where metrics are already covering. Let's reduce the amount of traces to events that are of interest -- the hottest of hottest paths (specifically Head request and get range by height, etc.) and only tracing the absolute most interesting events.
context is needed to create the baggage. And we will get a race if do not copy the variable. |
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.
🙏🏻
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.
Utack
Overview
Add traces for p2p.session