chore(storage): introduce storage trace package #11051
Conversation
storage/trace.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| OpenTelemetryTracingExpVar = "GO_STORAGE_EXPERIMENTAL_OTEL_TRACING" |
There was a problem hiding this comment.
I don't think we want to export this since we'd ideally remove it after the migration is done.
There was a problem hiding this comment.
Updated, thanks! If fuse needs it anytime sooner, we can change it to an experimental env var.
| return otel.Tracer(defaultTracerName, trace.WithInstrumentationVersion(internal.Version)) | ||
| } | ||
|
|
||
| // startSpan accepts SpanStartOption and is used to replace internal/trace/StartSpan. |
There was a problem hiding this comment.
This comment is fine for drafting, but we should change it to something else before merging
There was a problem hiding this comment.
Not seeing any changes here yet
storage/bucket.go
Outdated
| ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.Create") | ||
| defer func() { trace.EndSpan(ctx, err) }() | ||
| if isOTelTracingDevEnabled() { | ||
| ctx, _ = startSpan(ctx, "cloud.google.com/go/storage.Bucket.Create") |
There was a problem hiding this comment.
Since startSpan is completely internal to Storage now, can we factor out cloud.google.com/go/storage?
There was a problem hiding this comment.
Absolutely, I've changed these to storage.Bucket.Create so we're still following the semantic conventions of $package.$service.$method
There was a problem hiding this comment.
Oh I meant, just to move cloud.google.com/go/ to the startSpan method so that we don't have to repeat it in the name of each span (and to help with consistency).
tritone
changed the title
| ) | ||
|
|
||
| // isOTelTracingDevEnabled checks the development flag until experimental feature is launched. | ||
| func isOTelTracingDevEnabled() bool { |
There was a problem hiding this comment.
This func seems unused? What is the actual purpose of the env var?
There was a problem hiding this comment.
It will be used during development to enable the new stuff only if that var is set. It will be removed once launched
| storageOtelTracingDevVar = "GO_STORAGE_DEV_OTEL_TRACING" | ||
| defaultTracerName = "cloud.google.com/go/storage" | ||
| gcpClientRepo = "googleapis/google-cloud-go" | ||
| gcpClientArtifact = "storage" |
There was a problem hiding this comment.
Should this be cloud.google.com/go/storage or something like com.google.cloud.google-cloud-storage?
| func getCommonTraceOptions() []trace.SpanStartOption { | ||
| opts := []trace.SpanStartOption{ | ||
| trace.WithAttributes( | ||
| attribute.String("gcp.client.version", internal.Version), |
There was a problem hiding this comment.
Are we missing gcp.client.service?
| originalOtelTracingBool := os.Getenv("GO_STORAGE_DEV_OTEL_TRACING") | ||
| defer os.Setenv("GO_STORAGE_DEV_OTEL_TRACING", originalOtelTracingBool) | ||
|
|
||
| os.Setenv("GO_STORAGE_DEV_OTEL_TRACING", "true") |
There was a problem hiding this comment.
Instead of doing this, try https://pkg.go.dev/testing#T.Setenv
| originalOtelTracingBool := os.Getenv("GO_STORAGE_DEV_OTEL_TRACING") | |
| defer os.Setenv("GO_STORAGE_DEV_OTEL_TRACING", originalOtelTracingBool) | |
| os.Setenv("GO_STORAGE_DEV_OTEL_TRACING", "true") | |
| t.Setenv("GO_STORAGE_DEV_OTEL_TRACING", "true") |
| otel.SetTracerProvider(tp) | ||
|
|
||
| spanName := "storage.TestTrace.TestStorageTraceStartEndSpan" | ||
| spanStartopts := []trace.SpanStartOption{ |
There was a problem hiding this comment.
nit
| spanStartopts := []trace.SpanStartOption{ | |
| spanStartOpts := []trace.SpanStartOption{ |
|
|
||
| func spanAttributesComparer(a, b tracetest.SpanStub) bool { | ||
| if a.Name != b.Name { | ||
| fmt.Printf("name mismatch: a.Name: %v, b.Name: %v\n", a.Name, b.Name) |
There was a problem hiding this comment.
Shouldn't this be printed in the diff when comparing? What does the output on failure look like?
Introduce storage trace package, starting with the bucket module
GO_STORAGE_DEV_OTEL_TRACINGstartSpanaccepts StartSpanOption and can setAttributes