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

gotgelf · 2023-12-18T21:08:37Z

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

corhere

There is nothing about the instrumentation added to the filesystem driver in this PR which is specific to the filesystem driver. These spans should live in base.Base so that all drivers can benefit. 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.

corhere · 2023-12-19T18:10:28Z

registry/storage/driver/filesystem/driver.go

@@ -28,6 +31,10 @@ const (
 	minThreads = uint64(25)
 )

+// tracer is the OpenTelemetry tracer utilized for tracing operations within
+// this package's code.
+var tracer trace.Tracer


Suggested change

var tracer trace.Tracer

var tracer = otel.Tracer("github.com/distribution/distribution/v3/registry/storage/driver/filesystem")

corhere · 2023-12-19T18:36:18Z

registry/storage/driver/filesystem/driver.go

+	}
+	ctx, span := tracer.Start(
+		ctx,
+		fmt.Sprintf("%s:%s", driverName, "GetContent"),


I wonder if the driver name would be better encoded as a span attribute than as part of the span name. It would require fewer string concatenations at runtime when the tracing logic is lifted into base.Base, at least.

corhere · 2023-12-19T18:43:40Z

registry/storage/driver/filesystem/driver.go

@@ -118,6 +126,15 @@ func (d *driver) Name() string {

 // GetContent retrieves the content stored at "path" as a []byte.
 func (d *driver) GetContent(ctx context.Context, path string) ([]byte, error) {
+	attrs := []attribute.KeyValue{
+		attribute.String("Path", path),


https://opentelemetry.io/docs/specs/semconv/general/attribute-naming/

gotgelf · 2023-12-22T15:30:59Z

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?

corhere · 2023-12-22T16:31:03Z

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?

gotgelf · 2024-01-17T09:17:21Z

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!

milosgajdos · 2024-01-20T13:02:13Z

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 😞

corhere

The code itself looks good; it's just the span and attribute names which need some more attention.

corhere · 2024-01-25T00:25:10Z

registry/storage/driver/base/base.go

+	}
+	ctx, span := tracer.Start(
+		ctx,
+		"get_content",


The span name is not an attribute. The guidance is a little vague on the recommended format for span names in general, though it does mention the name of a function as an example choice of span name. I think we should name these spans after the function names exactly how they're spelled in code.

Suggested change

"get_content",

"GetContent",

Agreed, this was also on my mind. Utilizing the precise function names as span names significantly improves the clarity and consistency within our telemetry data. Moreover, it facilitates tracing by simplifying the process of correlating spans to the specific functions from which they originate.

corhere · 2024-01-25T00:28:38Z

registry/storage/driver/base/base.go

+		attribute.String("driver.name", base.Name()),
+		attribute.String("path", path),


https://opentelemetry.io/docs/specs/semconv/general/attribute-naming/#recommendations-for-application-developers

For new names, perhaps io.cncf.distribution would be a good choice of prefix?

gotgelf · 2024-02-09T16:18:54Z

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!

corhere

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

corhere · 2024-02-09T21:48:36Z

registry/registry.go

@@ -155,6 +155,7 @@ func NewRegistry(ctx context.Context, config *configuration.Configuration) (*Reg
 	}

 	err = tracing.InitOpenTelemetry(app.Context)
+


Stray newline snuck into a file that is not otherwise touched by this PR.

Suggested change

registry/storage/driver/base/base.go

corhere · 2024-02-09T22:01:53Z

registry/storage/driver/base/base.go

+	attrs := []attribute.KeyValue{
+		attribute.String(tracing.AttributePrefix+"driver.name", base.Name()),
+		attribute.String(tracing.AttributePrefix+"path", path),
+		attribute.Int("content.length", len(content)),


I don't see an attribute with that name in the OTEL semantic conventions. Did you mean to prefix it with tracing.AttributePrefix?

registry/storage/driver/base/base.go

gotgelf · 2024-02-10T08:59:49Z

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.

corhere

LGTM! 🎉

Thank you so much for working on tracing and putting up with all the review rounds. We really appreciate your efforts ❤️

corhere · 2024-02-14T15:39:16Z

registry/storage/driver/base/base.go

-	ctx, done := dcontext.WithTrace(ctx)
-	defer done("%s.GetContent(%q)", base.Name(), path)
+	attrs := []attribute.KeyValue{
+		attribute.String(tracing.AttributePrefix+"storage.driver.name", base.Name()),


Good call with making storage.driver a namespace ✅

milosgajdos

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

milosgajdos · 2024-03-04T08:06:17Z

go.mod

@@ -82,10 +82,10 @@ require (
 	go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.21.0 // indirect
 	go.opentelemetry.io/otel/exporters/prometheus v0.44.0 // indirect
 	go.opentelemetry.io/otel/exporters/stdout/stdoutmetric v0.44.0 // indirect
-	go.opentelemetry.io/otel/exporters/stdout/stdouttrace v1.21.0 // indirect
+	go.opentelemetry.io/otel/exporters/stdout/stdouttrace v1.21.0


Can you please move this direct dependency to the list of direct deps?

milosgajdos · 2024-03-04T08:06:30Z

go.mod

 	go.opentelemetry.io/otel/metric v1.21.0 // indirect
 	go.opentelemetry.io/otel/sdk/metric v1.21.0 // indirect
-	go.opentelemetry.io/otel/trace v1.21.0 // indirect
+	go.opentelemetry.io/otel/trace v1.21.0


Same comment as above

gotgelf · 2024-03-04T10:40:20Z

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.

milosgajdos · 2024-03-04T11:13:16Z

Hi @milosgajdos , it's done.

Thanks, one more thing, mind squashing @gotgelf ?

Signed-off-by: gotgelf <gotgelf@gmail.com>

gotgelf · 2024-03-04T12:37:57Z

Hi @milosgajdos , it's done.

Thanks, one more thing, mind squashing @gotgelf ?

Of course, done.

gotgelf changed the title ~~Added Open Telemetry Tracing to Filesystem package~~ [otel-tracing] Added Tracing to Filesystem package Dec 18, 2023

corhere requested changes Dec 19, 2023

View reviewed changes

corhere added enhancement area/storage labels Dec 19, 2023

gotgelf requested a review from corhere December 22, 2023 17:47

gotgelf changed the title ~~[otel-tracing] Added Tracing to Filesystem package~~ [otel-tracing] Added Tracing to Base package (driver) Dec 22, 2023

corhere reviewed Jan 25, 2024

View reviewed changes

github-actions bot added the dependencies Pull requests that update a dependency file label Feb 5, 2024

gotgelf force-pushed the otel-tracing-filesystem-instrumentation branch from 5b64082 to d973883 Compare February 5, 2024 17:45

gotgelf requested a review from corhere February 5, 2024 17:46

corhere reviewed Feb 9, 2024

View reviewed changes

gotgelf force-pushed the otel-tracing-filesystem-instrumentation branch from 5f73866 to f86fbdb Compare February 10, 2024 08:53

gotgelf requested a review from corhere February 10, 2024 08:55

corhere approved these changes Feb 14, 2024

View reviewed changes

corhere requested review from milosgajdos, deleteriousEffect, Jamstah and wy65701436 February 14, 2024 15:48

milosgajdos reviewed Mar 4, 2024

View reviewed changes

gotgelf requested a review from milosgajdos March 4, 2024 09:13

Added Open Telemetry Tracing to Filesystem package

f690b3e

Signed-off-by: gotgelf <gotgelf@gmail.com>

gotgelf force-pushed the otel-tracing-filesystem-instrumentation branch from 5fbedac to f690b3e Compare March 4, 2024 12:32

milosgajdos approved these changes Mar 4, 2024

View reviewed changes

milosgajdos merged commit 51a72c2 into distribution:main Mar 4, 2024

This was referenced Apr 21, 2024

Add opentracing support #1416

Closed

Feature：add trace features #3451

Closed

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

	var tracer trace.Tracer
	var tracer = otel.Tracer("github.com/distribution/distribution/v3/registry/storage/driver/filesystem")

		attribute.String("driver.name", base.Name()),
		attribute.String("path", path),

		@@ -155,6 +155,7 @@ func NewRegistry(ctx context.Context, config configuration.Configuration) (Reg
		}

		err = tracing.InitOpenTelemetry(app.Context)

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

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

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!