Tracing Refactor and Cleanup#135
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
@avalonche @ferranbt Could I grab a review? |
| if let Some(mut s) = span { | ||
| s.end() | ||
| }; | ||
| let _ = builder_client |
There was a problem hiding this comment.
I believe since the external payload id information is removed, there is an assumption here that the builder and local payload ids are the same. This is not always consistent across op-reth and op-geth?
There was a problem hiding this comment.
I believe payload ids in fact are always consistent actually. If they're not then it's a bug in reth/geth.
There was a problem hiding this comment.
the specs don't really specify how the payload id should be constructed
There was a problem hiding this comment.
Interesting. In practice they are the same at the very least.
|
@avalonche I believe I've addressed all your comments :) Please review my most recent 2 commits and let me know if everything looks good. |
avalonche
left a comment
avalonche
left a comment
There was a problem hiding this comment.
Changes look ok to me but please add outstanding comments as issues on the repo
|
also would be great to update the docs on the changes to metrics. I believe there are breaking changes to the metric labels which would break dashboards |
|
@avalonche I've updated the docs for you. Looks like I don't have permission to merge yet so you may have to do it for me! TY! |
…er/tracing Tracing Refactor and Cleanup
…er/tracing Tracing Refactor and Cleanup
3 participants
This PR contains a handful of improvements:
tracing-opentelemetrycrate