distribution icon indicating copy to clipboard operation
distribution copied to clipboard

[otel-tracing] Added Tracing to Base package (driver)

Open gotgelf opened this issue 1 year ago • 6 comments

This PR introduces OpenTelemetry tracing capabilities into the base(storage driver) component.

gotgelf avatar Dec 18 '23 21:12 gotgelf

Hi @corhere , thx for your review.

Feel free to rewrite dcontext.WithTrace or throw it out; OpenTelemetry tracing is superior and could also be wired up to log traces to logrus.

You mentioned the possibility of replacing dcontext.WithTrace with OpenTelemetry and wiring it up to log traces to logrus. I've explored this and have a question about maintaining our current logging format. Specifically, is it critical for our logrus logs to retain the same format as they currently have? For example:

DEBU[2330] filesystem.Stat("/") environment=development go.version=go1.21.4 instance.id=4d34dc87-5f21-4593-80ed-b959a8a923a0 service=registry trace.duration="82.583µs" trace.file=/Users/gotgelf/private/oss/distribution/registry/storage/driver/base/base.go trace.func="github.com/distribution/distribution/v3/registry/storage/driver/base.(*Base).Stat" trace.id=b6b807be-1a24-401a-99ef-d1a0989b45ca trace.line=152 version=v3.0.0+unknown

If maintaining this format is important, I believe developing a CustomExporter might be the best approach. This would give us direct access to the span data, including attributes, allowing us to log the information in any format we choose.

On the other hand, using the stdouttrace exporter presents a challenge in preserving our current log structure. The stdouttrace exporter outputs the entire span as a pre-formatted string, typically in JSON, which might not easily allow for parsing specific fields.

What are your thoughts on this? Would you prefer adherence to the current log format, or is there flexibility for change in the structure of our logs?

gotgelf avatar Dec 22 '23 15:12 gotgelf

What are your thoughts on this? Would you prefer adherence to the current log format, or is there flexibility for change in the structure of our logs?

It's a new major version, which gives us permission to make breaking changes. And I doubt anyone has written automation to parse the trace logs. If anyone has, I imagine they'd be thrilled to replace it with off-the-shelf OpenTelemetry tooling.

I had also considered suggesting a custom SpanExporter for logrus. You're welcome to implement one, and I'd be happy to review and see it merged, but I don't think we need to get that fancy. I personally would rather analyze a trace rendered graphically and/or including the registry client spans, i.e. in Jaeger, than to scroll throw a textual spew of raw spans. In my opinion, given the option for exporting traces over OTLP, the logrus trace output would be little more than a nice-to-have, a convenience for distribution developers to get a quick preview of traces when it would be overkill to launch a Jaeger all-in-one container.

What do you think @milosgajdos?

corhere avatar Dec 22 '23 16:12 corhere

Hi @corhere and @milosgajdos,

I hope this message finds you well. I wanted to follow up on my PR. I've addressed the comments received and requested another review. Since there hasn't been any activity for the past few weeks, I'm reaching out to ensure that this PR hasn't been overlooked.

If there are any additional changes or information needed from my side to move forward with the review process, please let me know. I'm eager to contribute and would appreciate any further guidance you can provide.

Thanks in advance!

gotgelf avatar Jan 17 '24 09:01 gotgelf

Sorry, I'm currently on sabbatical only accessing GH from my phone from random locations around the world so won't be able to give much proper feedback for a while I'm afraid 😞

milosgajdos avatar Jan 20 '24 13:01 milosgajdos

Hello @corhere,

Could you kindly review the latest updates in this pull request? I'm planning to open a new one that addresses #4275, so it would be preferable to close the current one beforehand.

Thank you!

gotgelf avatar Feb 09 '24 16:02 gotgelf

Almost there! And could you please squash down to a single commit so your PR branch is ready to merge?

@corhere Done! Thank you for the reviews and guidance.

gotgelf avatar Feb 10 '24 08:02 gotgelf

Thanks for this, can you please make the go.mod changes I've suggested? Then we're good to go.

Hi @milosgajdos , it's done.

gotgelf avatar Mar 04 '24 10:03 gotgelf

Hi @milosgajdos , it's done.

Thanks, one more thing, mind squashing @gotgelf ?

milosgajdos avatar Mar 04 '24 11:03 milosgajdos

Hi @milosgajdos , it's done.

Thanks, one more thing, mind squashing @gotgelf ?

Of course, done.

gotgelf avatar Mar 04 '24 12:03 gotgelf