Tag Nexus completion callback metrics with nexus_completion_source by tekkaya · Pull Request #11927 · temporalio/temporal

tekkaya · 2026-09-03T21:48:11Z

What changed?

Add an additional nexus_completion_source tag to currently emitted callback_outbound_requests and callback_outbound_latency metrics.
Improve callback completion logs with additional tags.

Why?

To distinguish nexus completion callback sources so that we can track what kind of durable execution is backing the nexus operation on the handler side.
To improve logging for better diagnose capabilities.

How did you test it?

Potential risks

It is a tag addition to existing metrics, will work with DevProd related to the tag addition.

github-actions · 2026-09-03T22:39:41Z

Claude finished @tekkaya's task in 2m 45s —— View job

Reviewed 10 files, 2 findings.
· gokhan/nexus-callback-completion-source-metric

github-actions · 2026-09-03T22:42:03Z


 type CompletionSource interface {
 	GetNexusCompletion(ctx chasm.Context, requestID string) (nexusrpc.CompleteOperationOptions, error)
+	GetNexusCompletionSourceVariant() string


nit — GetNexusCompletionSourceVariant uses a Get prefix and calls the value a "variant", which means something else in this codebase.

Repo Go conventions avoid the Get prefix on getters. GetNexusCompletion predates this and is constrained by the HSM CanGetNexusCompletion interface, but the new method is free to follow the convention.

"Variant" is also overloaded here: in the callback code it consistently refers to a proto oneof variant (callbackspb.Callback_Nexus_, "unprocessable callback variant: %v"). What this method returns is a metric tag value naming the component kind, which is what the interface name already says.

Same for the completionSourceVariant field on invocableOutbound (chasm/lib/callback/invocable_outbound.go:27) and the three implementations.

Suggestion: Rename to NexusCompletionSource() string (field: completionSource).

Suggested change

GetNexusCompletionSourceVariant() string

NexusCompletionSource() string

github-actions · 2026-09-03T22:42:05Z

+	if len(value) == 0 {
+		value = unknownValue
+	}


nit — The _unknown_ fallback is unreachable.

Every value reaching this tag is a compile-time constant: the three CompletionSource implementations return NexusCompletionSource{Workflow,WorkflowUpdate,Activity}TagValue, and nexus_invocation.go:77 passes the workflow constant literally. The neighboring tags in this file guard because they take dynamic or user-supplied values; this one can't be empty.

Suggestion: Drop the branch.

Suggested change

if len(value) == 0 {

value = unknownValue

}

I would consider adding a soft assert here because this really should never happen.

bergundy

Can you just double check the implication of adding a label to existing metrics?
I would also add what type of callback we are delivering: "nexus_handler", "nexus", "internal".

bergundy · 2026-09-04T17:01:40Z

+
 type CompletionSource interface {
 	GetNexusCompletion(ctx chasm.Context, requestID string) (nexusrpc.CompleteOperationOptions, error)
+	GetNexusCompletionSourceVariant() SourceVariant


We can return this via the same GetNexusCompletion method. You can do that in a follow up PR though. I want to refactor this interface though, there's code that should run in the callback executor that right now every completion source has to implement.

I'll do it in this PR 👍

Personally I'm a fan of adding another method to the CompletionSource interface, but also agree it should be refactoring since it's super clunky to use.

But rather than just returning the SourceVariant (or enumspb.ExecutionType), why not have it return the commonpb.Execution instead? (That is, the return value would also carry the execution_id and run_id values as well.)

That information can be obtained from the current chasm.Context. But (maybe in theory?) could differ. e.g. the Callback is CHASM execution X, and is delivering the Nexus result from CHASM execution Y? (Does that make sense @bergundy , or should this just be the execution type?)

The problem I see is that if we only return the execution type, we lose granularity. e.g. the callback would be associated with a Workflow instead of a Workflow Update...

Good callout @chrsmith!

You would need the attached component name (update) and root component name (activity, workflow). Those should be obtainable via CHASM context APIs.
Technically there's nothing limiting the callback to be attached to a component nested two levels deep in the tree, so if you really wanted to future proof this, you would need an API that will give you the full name of each component in the path and the execution archetype. That doesn't exist today but I would not go this far as long as we can support those cases in the future while keeping whatever labels we emit today stable.

workflow.updates/update workflow activity some-future-archetype.some-map/some-component.some-nested-map/some-nested-component

bergundy · 2026-09-04T17:27:37Z

+	if len(value) == 0 {
+		value = unknownValue
+	}


I would consider adding a soft assert here because this really should never happen.

tekkaya · 2026-09-04T17:42:23Z

Can you just double check the implication of adding a label to existing metrics? I would also add what type of callback we are delivering: "nexus_handler", "nexus", "internal".

I did that last night and apparently for internal metrics new tags to existing metrics appear in Prometheus / Thanos (internal alerts, Grafana) right away but if we want them in Chronicle / ClickHouse (analytics) we need to ask DevProd to do that. Since the completion source variant and the callback variant you suggested are low cardinality (both has 3 values) we should be able use them for alerting right away. Do you think the dimension I suggest in this PR and the callback variant you suggested will be useful for analytics as well? In that case I'll reach out to dev prod related to it

chrsmith

This is definitely something we want to do. But we might want to rework the way we are plumbing the data. (Using enumspb.ExecutinoType instead of a new CHASM-only enum, and thinking about how we'd update this when landing Worker Callbacks.)

I don't know how strongly I would push on these recommendations. (e.g. maybe adding another "callback-variant" tag wouldn't be a big deal?) So I'd defer to @bergundy.

chrsmith · 2026-09-04T17:36:48Z

 )

+// SourceVariant names the kind of component that produced a Nexus operation completion callback.
+type SourceVariant string


Let's not introduce a callback.SourceVariant and try to rely on the existing enums we have today. There already is a commonpb.Execution proto and enumspb.ExecutionType enum that already provide this information.

i.e. we'd just have SourceExecutionType enumspb.ExecutionType instead.

https://github.com/temporalio/api/blob/main/temporal/api/common/v1/message.proto#L73
https://github.com/temporalio/api/blob/main/temporal/api/enums/v1/common.proto#L110

You'll notice that, sadly, EXECUTION_TYPE_NEXUS_OPERATION. But we should add that additional value. (In fact, it's part of the Worker Callbacks API PR that hasn't been merged yet. temporalio/api#863)

enum ExecutionType { EXECUTION_TYPE_UNSPECIFIED = 0; // A workflow execution archetype. EXECUTION_TYPE_WORKFLOW = 1; // An activity execution archetype. This is reserved for standalone activities. EXECUTION_TYPE_ACTIVITY = 2; }

how do we distinguish between workflow and workflow_update using ExecutionType?

@chrsmith I realized the API change https://github.com/temporalio/api/pull/863/changes#diff-de60adc512f72d87dc1e38c0f30b41a16dede9243dfaf6209b4af9b5447fa551R312 doesn't support expressing a callback attached to a workflow update.

@bergundy , the plan was to use the component_id field. (Which admittedly isn't great, but will tide us over until we need another field like component_path.)

e.g. that PR, for the short term:

// For the short term. Link_Callback{ execution: {WORKFLOW, "wf-id-1", "run-id-1"}, component_id: "wf-update-id-1", } // Later, adding another field when something other than workflow updates // are setting the component_id. Link_Callback{ execution: {WORKFLOW, "wf-id-1", "run-id-1"}, component_id: "wf-update-id-1", component_path: ["workflow", "update"] // Or something like that. }

WDYT?

chrsmith · 2026-09-04T17:40:50Z

+
 type CompletionSource interface {
 	GetNexusCompletion(ctx chasm.Context, requestID string) (nexusrpc.CompleteOperationOptions, error)
+	GetNexusCompletionSourceVariant() SourceVariant


Personally I'm a fan of adding another method to the CompletionSource interface, but also agree it should be refactoring since it's super clunky to use.

But rather than just returning the SourceVariant (or enumspb.ExecutionType), why not have it return the commonpb.Execution instead? (That is, the return value would also carry the execution_id and run_id values as well.)

That information can be obtained from the current chasm.Context. But (maybe in theory?) could differ. e.g. the Callback is CHASM execution X, and is delivering the Nexus result from CHASM execution Y? (Does that make sense @bergundy , or should this just be the execution type?)

The problem I see is that if we only return the execution type, we lose granularity. e.g. the callback would be associated with a Workflow instead of a Workflow Update...

chrsmith · 2026-09-04T17:44:49Z

-	h.metricsHandler.Counter(RequestCounter.Name()).Record(1, namespaceTag, destTag, outcomeTag)
-	h.metricsHandler.Timer(RequestLatencyHistogram.Name()).Record(time.Since(startTime), namespaceTag, destTag, outcomeTag)
+	completionSourceTag := metrics.NexusCompletionSourceTag(string(n.completionSourceVariant))
+	h.metricsHandler.Counter(RequestCounter.Name()).Record(1, namespaceTag, destTag, outcomeTag, completionSourceTag)


One thing that we'll want to add shortly, is the variant of the callback used. Today we only support the Nexus-variant callback. But (🤞) SOON, there will be a new NexusHandler-variant. Adding another tag seems kinda lame.

So could we combine it? e.g.

// Activity_Nexus, Workflow_Nexus. And later NexusOperation_NexusHandler? completionInfo := `${completionSource}_${callbackVariant}` ``

Synced on this, decisions made:

will use component fully qualified name to distinguish the completion source

will add the callback variant as a separate tag

bergundy · 2026-09-04T17:59:58Z

Can you just double check the implication of adding a label to existing metrics? I would also add what type of callback we are delivering: "nexus_handler", "nexus", "internal".

I did that last night and apparently for internal metrics new tags to existing metrics appear in Prometheus / Thanos (internal alerts, Grafana) right away but if we want them in Chronicle / ClickHouse (analytics) we need to ask DevProd to do that. Since the completion source variant and the callback variant you suggested are low cardinality (both has 3 values) we should be able use them for alerting right away. Do you think the dimension I suggest in this PR and the callback variant you suggested will be useful for analytics as well? In that case I'll reach out to dev prod related to it

That maps to my understanding as well. I think it's worth just communicating our plan to add these limited cardinality labels to the prometheus backend ATM.

chrsmith

This code looks good to me, but if possible (and doesn't make the code worse off), I'd like you to look at a couple of things.

chrsmith · 2026-09-08T15:14:52Z


 type CompletionSource interface {
+	// A callback's parent (completion source) is always a CHASM component, see [chasm.ParentPtr.TryGet].
+	chasm.Component


It seems a little dicy to embed the entire interface. Since it kinda conflates "CompletionSource" (which is an already confusing interface used for getting the result of a Nexus operation) with a prototypical CHASM component.

What do you think about having this be available via a getter, instead? e.g.

type CompletionSource interface { SourceComponent() chasm.Component GetNexusCompletion(...) (...) }

Does that make the code cleaner? Or are we relying on it embedding the full interface for some reason?

Putting this behind a getter like SourceComponent() will still make it possible to access all accessible methods of the Component interface so I don't think we will gain from this change. One thing I will try to resolve the fqn from Node, instead of Component

Yes, having it behind SourceComponent() chasm.Component won't limit access in any way. But it will make any usages crystal clear.

e.g. without it, you could pass a CompletionSource to any location that would accept a chasm.Component, right? And that makes tracing things a little more difficult, as opposed to seeing all the places where the SourceComponent() method is called, and just looking there.

I wouldn't hold up this PR for that, it's just something I think we might want to consider since I'm guessing that we don't want to treat a CompletionSource as a regular CHASM component. And really just want a reliable way to extract metadata about where it came from.

chrsmith · 2026-09-08T15:16:51Z

 type invocableOutbound struct {
 	callback          *callbackspb.Callback_Nexus
 	completion        nexusrpc.CompleteOperationOptions
+	completionSource  string


Nit: Could you add comment here explaining what completionSource means? As a string, it isn't clear what the role is. Ultimately, it's just the metric tag we emit right? So maybe renaming this to completionSourceTag or sourceTag might be clearer?

I can do that, also it is not just used as a metric tag, it is also used as a log tag as well

chrsmith · 2026-09-08T15:19:44Z


+// componentFqn returns the fully qualified registered name of the given component, or "" if the
+// component is not registered.
+func (n *Node) componentFqn(component Component) string {


If the component is not registered, wouldn't it make more sense to return ("", errors.New("component %T is not registered with CHASM", ...))? i.e. failing loudly when things aren't working, instead of silently emitting potentially bad data.

tekkaya · 2026-09-08T23:55:22Z

Updated ptal @chrsmith

Add a nexus_completion_source tag to callback_outbound_requests and callback_outbound_latency, and a matching nexus-completion-source tag to the Nexus completion callback logs, identifying which component produced the completion: activity.activity, workflow.workflow or workflow.update. Without it that telemetry is an undifferentiated union of workflow-backed and activity-backed Nexus operation completions, so there is no way to tell how many Nexus operations are backed by standalone activities. The value is the completing component's fully qualified CHASM name rather than a hand-declared string, so it cannot drift from registration. A new ParentPtr.Fqn resolves it from the parent node's persisted type ID, which needs no Context and does not deserialize the parent; the tree traversal it shares with TryGet is factored out into parentNode. HSM callbacks are always workflow-backed and report chasm.WorkflowArchetype directly. The failure logs also gain the namespace, execution, and attempt context they previously lacked. Log messages are unchanged so existing log-based alerting keeps matching.

chrsmith

An optional nitpick, but just go ahead and merge it so I can rebase and fix this in the pending PRs for worker callbacks.

chrsmith · 2026-09-09T17:54:40Z

+	// completionSourceTag is the fully qualified name of the CHASM component that produced this
+	// completion, e.g. "workflow.workflow" or "activity.activity".
+	completionSourceTag string
+	workflowID, runID   string


Nit: No need for another rev of the PR, but I'll rename workflowId to businessId. Since the callback might be attached to something other than a Workflow. (And "business ID" is the catch all instead of { activity, workflow, operation, ... } ID.)

rodrigozhou · 2026-09-09T21:27:31Z

-	h.metricsHandler.Timer(RequestLatencyHistogram.Name()).Record(time.Since(startTime), namespaceTag, destTag, outcomeMetricTag)
+	completionSourceMetricTag := metrics.NexusCompletionSourceTag(n.completionSourceTag)
+	h.metricsHandler.Counter(RequestCounter.Name()).Record(1, namespaceTag, destTag, outcomeMetricTag, completionSourceMetricTag)
+	h.metricsHandler.Timer(RequestLatencyHistogram.Name()).Record(time.Since(startTime), namespaceTag, destTag, outcomeMetricTag, completionSourceMetricTag)


Unrelated to your changes, but probably more accurate to store time.Since(startTime right after L75.

rodrigozhou · 2026-09-09T21:33:21Z

+		return rc.fqType()
+	}
+
+	softassert.Fail(parent.logger, "parent component type is not registered with CHASM")


Should we panic instead if this is meant to never happen?

rodrigozhou · 2026-09-09T21:34:26Z

-	e.MetricsHandler.Timer(RequestLatencyHistogram.Name()).Record(time.Since(startTime), namespaceTag, destTag, statusCodeTag)
+	completionSourceTag := metrics.NexusCompletionSourceTag(chasm.WorkflowArchetype)
+	e.MetricsHandler.Counter(RequestCounter.Name()).Record(1, namespaceTag, destTag, statusCodeTag, completionSourceTag)
+	e.MetricsHandler.Timer(RequestLatencyHistogram.Name()).Record(time.Since(startTime), namespaceTag, destTag, statusCodeTag, completionSourceTag)


ditto, unrelated but ideally, time.Since should be right after L74

yycptt

Some minor clarification questions. Nothing blocking given this is only for metric tags.

yycptt · 2026-09-09T22:58:52Z

-	if !isT {
-		// nolint:forbidigo // Panic is intended here for framework error handling.
-		panic(serviceerror.NewInternalf("parent component value doesn't implement %s", reflect.TypeFor[T]().Name()))
+	if typeID := parent.serializedNode.GetMetadata().GetComponentAttributes().GetTypeId(); typeID != 0 {


when will typeID be 0? is it for a newly created node and the code only performed syncStructure, but not the serialization part yet?

Yes, TypeId is written on first serialization, in serializeComponentNode, so a node created in the current transaction reads 0 until CloseTransaction serializes it. typeID being 0 reachable in this scenario:

CloseTransaction runs executeImmediatePureTasks first, and syncSubComponents, which initializes ParentPtr.currentNode runs before those tasks, while closeTransactionSerializeNodes runs after. So an immediate pure task can hold an initialized ParentPtr whose parent has no TypeId yet. The fallback is trying to cover this case.

yycptt · 2026-09-09T22:59:20Z

+		}
 	}
-	return vT, true
+	if rc, ok := parent.registry.componentFor(parent.value); ok {


do we need to call prepareComponentValue before access parent.value?

We do, currently this new Fqn method is only called from loadInvocationArgs method of the callback component and that method first makes a Get call to ParentPtr which makes the prepareComponentValue call. But you are right that Fqn can not rely on Get being called first, I'll move the prepareComponentValue to the new parentNode method

Correction to my earlier reply, we don't need it. prepareComponentValue resolves the type via registry.ComponentByID(TypeId), we do the same lookup in Fqn method already: if TypeId resolves, branch 1 has already returned the name; if it doesn't, prepareComponentValue fails too.

Will add a comment in the followup PR to clarify this.

tekkaya changed the title ~~Tag Nexus completion callback metrics with completion_source~~ Tag Nexus completion callback metrics with nexus_completion_source Sep 3, 2026

tekkaya force-pushed the gokhan/nexus-callback-completion-source-metric branch from ce0867f to 01f68a6 Compare September 3, 2026 21:56

davidporter-id-au approved these changes Sep 3, 2026

View reviewed changes

tekkaya marked this pull request as ready for review September 3, 2026 22:39

tekkaya requested a review from a team September 3, 2026 22:39

tekkaya requested review from a team as code owners September 3, 2026 22:39

github-actions Bot reviewed Sep 3, 2026

View reviewed changes

tekkaya requested a review from bergundy September 3, 2026 22:42

tekkaya force-pushed the gokhan/nexus-callback-completion-source-metric branch 2 times, most recently from d29b77e to 2af1f48 Compare September 4, 2026 01:28

bergundy reviewed Sep 4, 2026

View reviewed changes

chrsmith self-assigned this Sep 4, 2026

chrsmith reviewed Sep 4, 2026

View reviewed changes

tekkaya force-pushed the gokhan/nexus-callback-completion-source-metric branch from 2af1f48 to 3e5f82d Compare September 5, 2026 06:03

tekkaya requested a review from a team as a code owner September 5, 2026 06:03

tekkaya force-pushed the gokhan/nexus-callback-completion-source-metric branch 2 times, most recently from dfaa39c to 5794a42 Compare September 5, 2026 06:15

tekkaya requested review from bergundy and chrsmith September 6, 2026 04:32

chrsmith reviewed Sep 8, 2026

View reviewed changes

tekkaya force-pushed the gokhan/nexus-callback-completion-source-metric branch from 5794a42 to 7116f3e Compare September 8, 2026 23:54

tekkaya requested a review from chrsmith September 8, 2026 23:55

tekkaya force-pushed the gokhan/nexus-callback-completion-source-metric branch from 7116f3e to 6deb9e0 Compare September 9, 2026 17:39

chrsmith approved these changes Sep 9, 2026

View reviewed changes

tekkaya enabled auto-merge (squash) September 9, 2026 18:23

yycptt requested review from awln-temporal and rodrigozhou September 9, 2026 18:45

rodrigozhou approved these changes Sep 9, 2026

View reviewed changes

tekkaya merged commit 07050f8 into main Sep 9, 2026
90 of 94 checks passed

tekkaya deleted the gokhan/nexus-callback-completion-source-metric branch September 9, 2026 21:34

yycptt reviewed Sep 9, 2026

View reviewed changes

	GetNexusCompletionSourceVariant() string
	NexusCompletionSource() string

Conversation

tekkaya commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed?

Why?

How did you test it?

Potential risks

Uh oh!

github-actions Bot commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergundy left a comment

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!

tekkaya commented Sep 4, 2026

Uh oh!

chrsmith left a comment

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!

Choose a reason for hiding this comment

Uh oh!

bergundy commented Sep 4, 2026

Uh oh!

chrsmith left a comment

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!

tekkaya commented Sep 8, 2026

Uh oh!

chrsmith left a comment

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

tekkaya commented Sep 3, 2026 •

edited

Loading

github-actions Bot commented Sep 3, 2026 •

edited

Loading

tekkaya Sep 9, 2026 •

edited

Loading