[otel-tracing] Added Tracing to Base package (driver)
This PR introduces OpenTelemetry tracing capabilities into the base(storage driver) component.
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?
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?
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!
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 😞
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!
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.
Thanks for this, can you please make the
go.modchanges I've suggested? Then we're good to go.
Hi @milosgajdos , it's done.
Hi @milosgajdos , it's done.
Thanks, one more thing, mind squashing @gotgelf ?
Hi @milosgajdos , it's done.
Thanks, one more thing, mind squashing @gotgelf ?
Of course, done.