Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

SamYuan1990
Copy link
Contributor

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]

Signed-off-by: Sam Yuan <[email protected]>
@denyeart
Copy link
Contributor

denyeart commented Jan 5, 2022

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.

@atoulme
Copy link
Contributor

atoulme commented Jan 5, 2022

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.

@SamYuan1990
Copy link
Contributor Author

SamYuan1990 commented Jan 7, 2022

Hi @atoulme, my idea is that use OpenTelemetry/OpenTracing to trace any Tx, I think we have the same goal here.
into detail level,
well, yes, OpenTelemetry/OpenTracing has their own hash logic, and they searched/index the spans etc base on the hash record generated in OpenTelemetry/OpenTracing.

I am not sure if we are able to any of follow things to archive the goal?

  • OpenTelemetry/OpenTracing has any way to support as "bring your own hash", as we can see blockchain as fabric will has hash as Tx hash and block hash. so... if we can input Tx hash as OpenTelemetry/OpenTracing id. For any Tx with a Tx hash, people can search the Tx id in OpenTelemetry/OpenTracing.
  • If we have two hash exists, and we put Tx id as business hash value as an attribute, is there any way for us to search the span id by attribute value.
  • A 3rd party system analysis OpenTelemetry/OpenTracing spans and convert Tx id to span ID....

@atoulme
Copy link
Contributor

atoulme commented Jan 19, 2022

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.

@SamYuan1990
Copy link
Contributor Author

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

txid=31a230b23c67f854ec4e9505283270055e16259d0a07e210f27cb8decb64d086

I just find how to use it in screen shot of jaegertracing/jaeger#2540

For ex:
image

@SamYuan1990
Copy link
Contributor Author

@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?

@denyeart
Copy link
Contributor

denyeart commented Feb 7, 2022

The conversation here makes sense. The actual RFC text update should be updated to reflect the conversation.

@SamYuan1990
I'd suggest to start over based on the original RFC text and add a couple minimal points. No need for all the links etc as they do not age well in a merged RFC.

@SamYuan1990
Copy link
Contributor Author

The conversation here makes sense. The actual RFC text update should be updated to reflect the conversation.

@SamYuan1990 I'd suggest to start over based on the original RFC text and add a couple minimal points. No need for all the links etc as they do not age well in a merged RFC.

Hi @denyeart , I made some updates in this PR. Please review and let me know if I missing any thing important. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants