fabric-rfcs icon indicating copy to clipboard operation
fabric-rfcs copied to clipboard

update open tracing rfc

Open SamYuan1990 opened this issue 2 years ago • 8 comments

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]

SamYuan1990 avatar Dec 06 '21 12:12 SamYuan1990

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.

denyeart avatar Jan 05 '22 02:01 denyeart

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.

atoulme avatar Jan 05 '22 22:01 atoulme

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.
  • [x] 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....

SamYuan1990 avatar Jan 07 '22 12:01 SamYuan1990

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.

atoulme avatar Jan 19 '22 17:01 atoulme

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 https://github.com/jaegertracing/jaeger/issues/2540

For ex: image

SamYuan1990 avatar Jan 23 '22 06:01 SamYuan1990

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

SamYuan1990 avatar Jan 23 '22 06:01 SamYuan1990

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.

denyeart avatar Feb 07 '22 20:02 denyeart

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. :-)

SamYuan1990 avatar Feb 08 '22 02:02 SamYuan1990