Repository navigation
Conversation
6922a92 to
4796387
Compare
| if allAttrsEmpty(resources.Set(), semconv.ServiceInstanceIDKey, semconv.FaaSInstanceKey, semconv.HostIDKey) { | ||
| instanceID, err := hostname() | ||
| if err != nil { | ||
| slog.ErrorContext(ctx, "os.Hostname() failed, setting OTel service instance ID to UUID", slog.Any("error", err)) |
| return nil, err | ||
| } | ||
| // Set an instance ID if the detectors and env extractor above didn't come up with one. | ||
| if allAttrsEmpty(resources.Set(), semconv.ServiceInstanceIDKey, semconv.FaaSInstanceKey, semconv.HostIDKey) { |
There was a problem hiding this comment.
Is it possible for an InstanceID to be set without any of these resources being set? This looks a bit brittle at first sight and seems to be CloudRun specific, so maybe it calls for a comment and could be extracted in its own function?
There was a problem hiding this comment.
I don't think so, no: the first of these (semconv.ServiceInstanceIDKey) is the key which carries the service instance ID: https://pkg.go.dev/go.opentelemetry.io/otel/semconv/v1.20.0#ServiceInstanceIDKey
So this condition is only true if the detectors/env didn't come up with any sort of local instance identifier (i.e an explicit instance ID, a Cloud Run style instance ID, or a host ID key).
There was a problem hiding this comment.
the first of these (semconv.ServiceInstanceIDKey) is the key which carries the service instance ID
Is that the case for CloudRun? My understanding chatting with Gemini is that it's in FaaSInstanceKey (but I might be wrong).
Also if OpenTelemetry changed how this works, this code might now work as intended anymore. Otel could for instance remove one of these keys, or introduce a new one.
I'm not sure we can write better code, but maybe we could add comments to explain that:
ServiceInstanceIDKeyis the default field, which can be set with an environment variable?FaaSInstanceKeyis for CloudRunHostIDKeyis for GCE- GKE and more are not supported
(That's the conclusion that I came to chatting with Gemini about this.)
Ideally, there would be a way to fetch these keys from Otel / to check whether the Instance ID has been set... but I'm not sure whether we can do that.
Approving since the priority is to unblock CloudRun, and this PR does that. We can polish later.
4796387 to
13b9476
Compare
This PR improves the exporting of OTel metrics when running under Cloud Run.
Previously, Cloud Run v2 would always show an instance ID of
localhost.