-
Notifications
You must be signed in to change notification settings - Fork 44
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
update open tracing rfc #47
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Sam Yuan <[email protected]>
Thanks @SamYuan1990 . I agree on the points around txid, that seems like a natural trace point across components. Yacov and I suggested the same thing in the original RFC review but it never made it into the final RFC. Let's ask the original author @atoulme to provide detailed responses to your additions, and understand if there was a reason why txid was not mentioned in the original RFC. |
The txid can be added to the trace as an attribute, but is not a valid identifier as the trace can originate outside Fabric. OpenTelemetry/OpenTracing have created systems to coordinate and correlate traces based on trace ID/parent trace ID/span ID, and are better suited for correlation. |
Hi @atoulme, my idea is that use OpenTelemetry/OpenTracing to trace any Tx, I think we have the same goal here. I am not sure if we are able to any of follow things to archive the goal?
|
It’s best to leave the id of the trace or span to the system generating the trace, especially as traces will not just originate from Fabric. The trace should contain attributes such as the transaction metadata so you can query by transaction id. |
aha, for ex, we can searched by tx id as sample below
I just find how to use it in screen shot of jaegertracing/jaeger#2540 |
Signed-off-by: Sam Yuan <[email protected]>
@denyeart , as comments above, verified with current POC, we are able to do a "end to end" transaction tracing by tag "txid=$txid". could you please help add reviewers in this PR and I hope to see their comments, if everyone good with this PR, let's merge it and move to implementation phase? |
The conversation here makes sense. The actual RFC text update should be updated to reflect the conversation. @SamYuan1990 |
Signed-off-by: Sam Yuan <[email protected]>
Hi @denyeart , I made some updates in this PR. Please review and let me know if I missing any thing important. :-) |
Hi Fabric,
Recently, I am adding open tracing for Tape specific for issue
It looks like if we adding open tracing in fabric process, it may help us understand bottleneck in fabric process better?
That's the reason I updated open telemetry related rfc.
Signed-off-by: Sam Yuan [email protected]