feat(otel): set ate.* actor telemetry identity on ateapi spans#412
feat(otel): set ate.* actor telemetry identity on ateapi spans#412Krisztian F (krisztianfekete) wants to merge 4 commits into
Conversation
776de1e to
5ba9da5
Compare
| if err := validatePauseActorRequest(req); err != nil { | ||
| return nil, err | ||
| } | ||
| setSpanActorRefIdentity(ctx, req.GetActor().GetAtespace(), req.GetActor().GetName()) |
There was a problem hiding this comment.
out of curiosity, what will be happened if instead of setSpanActorRefIdentity, the "setSpanActorIdentity" will be called.
The only value that is changing between line 33 to 46 is version.
I guess the intention is to keep the latest version right? Is it possible to call to setSpanActorIdentity twice. before s.actorWorkflow.PauseActor and after? The latest version will be submitted in successful case and previous version will be submitted in case of error.
There was a problem hiding this comment.
We can't call the full one early. At line 33 we only have the ObjectRef from the request and setSpanActorIdentity needs the resolved *Actor that we don't have until the workflow returns it.
Attributes overwrite by key, so the later wins and we get the latest version on success. On error we return early with only the ref, so no version.
I guess we could also add the version on mid-workflow failures if we move it inside the workflow. Maybe that's better as a follow-up if it's worth it. What do you think?
There was a problem hiding this comment.
I'd keep it as ate.actor.version. It means the same thing on every op, which is the version the op resulted in. Forking it to new_version on pause but version on create breaks querying for spans that have actor A at version X.
Happy to add a line to the attr doc spelling out "resulting version" or similar if that makes it better?
What do you mean by it? |
Thanks for the review! |
|
|
||
| // setSpanActorRefIdentity is setSpanActorIdentity for the identity subset known | ||
| // before the Actor record resolves, so a failed lookup still carries who/where. | ||
| func setSpanActorRefIdentity(ctx context.Context, atespace, actorID string) { |
There was a problem hiding this comment.
the parameter name is actorID but the callsites are passing in actor name, can we change it to actorName?
There was a problem hiding this comment.
Fair, but this turned out to be a bit deeper when started looking into it. ate.actor.id actually had the name while we also have a real uid. Went full k8s-style as per OTel semconv: the attr is now ate.actor.name and I added ate.actor.uid.
| // setSpanActorRefIdentity is setSpanActorIdentity for the identity subset known | ||
| // before the Actor record resolves, so a failed lookup still carries who/where. | ||
| func setSpanActorRefIdentity(ctx context.Context, atespace, actorID string) { | ||
| trace.SpanFromContext(ctx).SetAttributes(ateattr.ActorRefIdentity(atespace, actorID)...) |
There was a problem hiding this comment.
Similarly here, can we rename ActorRefIdentity to ActorObjectReference to be consistent with pkg/proto/ateapipb/ateapi.proto
There was a problem hiding this comment.
I went with ActorRefAttributes instead of ActorObjectReference, as it returns attributes, not a reference object, and it matches ActorAttributes. What do you think?
| } | ||
|
|
||
| // ActorIdentity is nil-safe; a nil Actor yields zero-valued attributes. | ||
| func ActorIdentity(a *ateapipb.Actor) []attribute.KeyValue { |
There was a problem hiding this comment.
Nit: I think this should be ActorAttributes since it logs several fields that I would not consider as the "identity" of the actor.
There was a problem hiding this comment.
This is now done.
| }) | ||
|
|
||
| assertSpanStr(t, attrs, ateattr.AtespaceKey, testAtespace) | ||
| assertSpanStr(t, attrs, ateattr.ActorIDKey, "id1") |
There was a problem hiding this comment.
nit: also create constants for "id" similar to testAtespace above?
There was a problem hiding this comment.
This is done, added testActorID.
| if err := validatePauseActorRequest(req); err != nil { | ||
| return nil, err | ||
| } | ||
| setSpanActorRefIdentity(ctx, req.GetActor().GetAtespace(), req.GetActor().GetName()) |
| // Dotted ate.* matches the metric-instrument naming (atenet.*, atelet.*), not the | ||
| // ate.dev/ slash form used for k8s labels and stdout log fields. | ||
| const ( | ||
| AtespaceKey = attribute.Key("ate.atespace") |
There was a problem hiding this comment.
ate.actor.atespace?
There was a problem hiding this comment.
I think I prefer ate.atespace. Atespace is the tenant/isolation boundary, where its own resource, templates is too), so if it's a top level attr you can filter spans by tenants regardless of resource type. It's the same reason OTel uses k8s.namespace.name, not k8s.pod.namespace. What do you think?
There was a problem hiding this comment.
Add a test in pause_actor_test.go?
There was a problem hiding this comment.
Makes sense, added this!
| "github.com/agent-substrate/substrate/pkg/proto/ateapipb" | ||
| ) | ||
|
|
||
| // installSpanRecorder swaps in a recording global TracerProvider. Span-producing |
There was a problem hiding this comment.
Let's have each test create its own trace provider to avoid this global provider shared by all tests? We will want to run tests in parallel later to speed up testing.
There was a problem hiding this comment.
Great idea, added this, thanks!
|
While reviewing this I realized today the atenet router uses different keys as trace attribute and the traces are not connected with the ateapi one, for the same ResumeActor request, we have one trace which traces the request through AteAPI-Atelet -Ateom but another, detached one with just the atenet router: We don't need to address it in this PR. Could you help us create a tracking issue for this? |
Yep, I am not addressing the router keys here, opened this one to track it (happy to pick it up as well): #427 |
…n/metric attrs Fixes trace detachment in the router where two issues broke the trace chain: 1. Primary: resumer.go's singleflight used context.Background() inside DoChan, so otelgrpc saw no parent span and ateapi handler spans appeared in a separate trace. Now propagates the caller's span context into the background context. 2. Secondary: the ext_proc HeadersResponse only rewrote :authority and never injected traceparent/tracestate into the header mutation. Now injects the trace context so the upstream worker receives the same trace even when Envoy tracing is not configured. 3. Span and metric attributes used bare keys (atespace, actor, etc.) instead of the ate.* convention established in agent-substrate#412. Renamed to ate.atespace, ate.actor_name, ate.target_addr, ate.actor_template_namespace, ate.actor_template_name, ate.outcome. Fixes agent-substrate#427
…n/metric attrs Fixes trace detachment in the router where two issues broke the trace chain: 1. Primary: resumer.go's singleflight used context.Background() inside DoChan, so otelgrpc saw no parent span and ateapi handler spans appeared in a separate trace. Now propagates the caller's span context into the background context. 2. Secondary: the ext_proc HeadersResponse only rewrote :authority and never injected traceparent/tracestate into the header mutation. Now injects the trace context so the upstream worker receives the same trace even when Envoy tracing is not configured. 3. Span and metric attributes used bare keys (atespace, actor, etc.) instead of the ate.* convention established in agent-substrate#412. Renamed to ate.atespace, ate.actor_name, ate.target_addr, ate.actor_template_namespace, ate.actor_template_name, ate.outcome. Fixes agent-substrate#427
…n/metric attrs Fixes trace detachment in the router where two issues broke the trace chain: 1. Primary: resumer.go's singleflight used context.Background() inside DoChan, so otelgrpc saw no parent span and ateapi handler spans appeared in a separate trace. Now propagates the caller's span context into the background context. 2. Secondary: the ext_proc HeadersResponse only rewrote :authority and never injected traceparent/tracestate into the header mutation. Now injects the trace context so the upstream worker receives the same trace even when Envoy tracing is not configured. 3. Span and metric attributes used bare keys (atespace, actor, etc.) instead of the ate.* convention established in agent-substrate#412. Renamed to ate.atespace, ate.actor_name, ate.target_addr, ate.actor_template_namespace, ate.actor_template_name, ate.outcome. Fixes agent-substrate#427
…n/metric attrs Two bugs broke the trace chain through the router, creating two separate traces instead of one connected trace for each ResumeActor request. 1. Singleflight detaches trace context (primary): resumer.go used context.Background() inside singleflight.DoChan(). Fixed by propagating the caller's span context into the background context via trace.ContextWithSpanContext. 2. No traceparent injected into upstream request (secondary): extproc.go only rewrote the :authority header. Fixed by calling injectTraceContext(ctx, mutation) which uses otel.GetTextMapPropagator() to write traceparent/tracestate with OVERWRITE_IF_EXISTS_OR_ADD AppendAction. 3. Non-namespaced attribute keys: Renamed span and metric attributes to ate.* convention (agent-substrate#412 style). Replaced custom target_addr attribute with stable OTel semconv server.address + server.port. Co-authored-by: krisztianfekete <krisztian.fekete@example.com>
…n/metric attrs Two bugs broke the trace chain through the router, creating two separate traces instead of one connected trace for each ResumeActor request. 1. Singleflight detaches trace context (primary): resumer.go used context.Background() inside singleflight.DoChan(). Fixed by propagating the caller's span context into the background context via trace.ContextWithSpanContext. 2. No traceparent injected into upstream request (secondary): extproc.go only rewrote the :authority header. Fixed by calling injectTraceContext(ctx, mutation) which uses otel.GetTextMapPropagator() to write traceparent/tracestate with OVERWRITE_IF_EXISTS_OR_ADD AppendAction. 3. Non-namespaced attribute keys: Renamed span and metric attributes to ate.* convention (agent-substrate#412 style). Replaced custom target_addr attribute with stable OTel semconv server.address + server.port.





This PR sets consistent, namespaced actor-identity set on the spans we already emit:
ate.atespace,ate.actor.id,ate.actor.template.{name,namespace},ate.actor.versionWe do this, so platform traces are queryable by actor, atespace, and template. Cross-hop correlation and per-tenant/per-template filtering, all on traces (not TSDB labels, as discussed in #174). This is a general, workload-neutral telemetry identity plumbing. We could add gen_ai specific stuff on top of this later.
This is a resume:

Notes:
Left as it was on purpose: metric attribute names and the stdout log labels (
ate.dev/*), but we should probably do this as well. Maybe in this PR?Bikeshed is welcomed on the
ate.*spelling before merge, renaming span attrs later is painful.Tests pass
Appropriate changes to documentation are included in the PR