Tracing support improvements #1819

siavashkavousi · 2023-07-16T20:38:30Z

Improve support for tracing signals, queries, and updates.

Our team encountered issues with tracing not being supported across workflow signals, queries, and updates when using temporal. I am proposing a proof of concept to address this, and if it is accepted, I will handle the necessary improvements and tests.

Note:
I have manually tested this in our test environment, and it appears to be working. However, as mentioned earlier, I will add tests after the initial review.

CLAassistant · 2023-07-16T20:38:36Z

All committers have signed the CLA.

cretz

Looks great and seems to mostly match our other SDKs. Will let @Quinn-With-Two-Ns and otehrs review.

cretz · 2023-07-17T12:17:15Z

...ing/src/main/java/io/temporal/opentracing/internal/ActionTypeAndNameSpanBuilderProvider.java

        String runId = context.getRunId();
        Preconditions.checkNotNull(
            runId, "runId is expected to be not null for span operation type %s", operationType);
        return ImmutableMap.of(
            StandardTagNames.WORKFLOW_ID, context.getWorkflowId(),
            StandardTagNames.RUN_ID, context.getRunId());
+      case QUERY_WORKFLOW:
+        return ImmutableMap.of();


In our other SDKs, query spans also have workflow ID and run ID.

The problem here is that handleQuery is not executed within a workflow thread, leading to non-deterministic errors.

Suggested change

return ImmutableMap.of();

return ImmutableMap.of(

StandardTagNames.WORKFLOW_ID, context.getWorkflowId(),

StandardTagNames.RUN_ID, context.getRunId());

To confirm, are you saying change this to the above errors?

I see, you are not setting the workflow ID and run ID on HANDLE_QUERY. You should set those the same way you do for HANDLE_SIGNAL.

I'm not sure that a span without these tags is ok, but I'll let y'all decide

Well the options are either no span or a span without those tags. I have no preference.

Regardless of which we go with we will need to open an issue to address this when we fix getInfo in query handlers , but I don't think that should block this PR.

Concur, will let you make the call on tagless span vs no span. I don't have a super-strong opinion here.

I believe it would be better to have a tagless span since the span itself is useful for tracing.

Works for me

cretz · 2023-07-17T12:18:37Z

...rc/main/java/io/temporal/opentracing/internal/OpenTracingWorkflowClientCallsInterceptor.java

+                        tracer,
+                        input.getSignalName(),
+                        input.getWorkflowExecution().getWorkflowId(),
+                        References.FOLLOWS_FROM)


I think the workflow inbound should make this a follows-from, but client outbound should be a child-of

cretz · 2023-07-17T12:21:03Z

...c/main/java/io/temporal/opentracing/internal/OpenTracingWorkflowInboundCallsInterceptor.java

+        spanFactory
+            .createWorkflowSignalSpan(
+                tracer,
+                input.getSignalName(),


I recommend, to match other SDKs, that workflow inbound signal, query, and update span names be HandleSignal:<name>, HandleQuery:<name>, and HandleQuery:<name> instead of reusing the same outbound names

cretz · 2023-07-17T12:23:03Z

...c/main/java/io/temporal/opentracing/internal/OpenTracingWorkflowInboundCallsInterceptor.java

+        contextAccessor.readSpanContextFromHeader(input.getHeader(), tracer);
+    Span workflowSignalSpan =
+        spanFactory
+            .createWorkflowSignalSpan(


I didn't read this closely, but what many of our other SDKs do is make this span a child-of the original inbound workflow-level span and a follows-from the inbound signal-level span. Is that what's happening here?

can you elaborate more on this (a link, doc or something related to other SDKs)?

When you view this HandleSignal span in many SDKs, its parent is the same as the RunWorkflow's parent (i.e. the span that came in the original start workflow header). And this HandleSignal span is just linked via follows-from to the span in this call's header.

cretz · 2023-07-25T13:31:43Z

temporal-opentracing/src/main/java/io/temporal/opentracing/internal/SpanFactory.java

+    return createSpan(context, tracer, null, References.FOLLOWS_FROM);
+  }
+
+  public Tracer.SpanBuilder createWorkflowSignalSpan(


Suggested change

public Tracer.SpanBuilder createWorkflowSignalSpan(

public Tracer.SpanBuilder createWorkflowHandleSignalSpan(

Can you change the names for handle-based spans instead of overloads where the caller can't know they are creating different operation types just by calling different overloads?

Quinn-With-Two-Ns · 2023-08-17T15:32:00Z

hm looks like opentracing.SignalWithStartTest test if failing, does the test need to be updated?

Quinn-With-Two-Ns · 2023-08-31T20:40:53Z

@siavashkavousi looks like opentracing.SignalWithStartTest::signalWithStart is still failing in CI, do you understand why?

siavashkavousi · 2023-09-01T21:25:52Z

@siavashkavousi looks like opentracing.SignalWithStartTest::signalWithStart is still failing in CI, do you understand why?

@Quinn-With-Two-Ns I'm sorry; I didn't have the time to investigate. I've taken a look at the tests now, and it seems that with our support for signal spans, we should update the assertions there. Specifically, we now expect two spans as children of the parent span. It is weird that unit tests without Docker are now failing. I'll investigate this issue further in the coming days to understand it in more detail.

siavashkavousi · 2023-09-09T22:09:27Z

@Quinn-With-Two-Ns I have reviewed the situation, and it appears that the issue arises from the disparity between in-memory tests and Docker integration tests. To address this, I used SDKTestWorkflowRule.useExternalService to accommodate distinct assertions for these two environments, thus preventing the need for further modifications. I propose that we address the in-memory test environment bug in a separate pull request. what do you suggest?

Quinn-With-Two-Ns · 2023-09-09T22:58:05Z

@siavashkavousi That is fine by me. The in memory test server probably does not pass headers through in some situations . For CI to pass, unfortunately some of our CI requires that when you pull the repo you need to use --tags can you update your branch? Sorry about the inconvenience.

…mprovements

Quinn-With-Two-Ns · 2023-09-15T06:01:19Z

@siavashkavousi some of our CI requires that when you pull the repo you need to use --tags can you update your branch so we CI can pass and we can merge your contribution?

siavashkavousi · 2023-09-19T13:59:22Z

@Quinn-With-Two-Ns I apologize for the delay. I've pulled and pushed tags. Could you please verify if I've done the right thing or not, please?

Quinn-With-Two-Ns

@siavashkavousi Thanks for the contribution, I'll leave open for a couple days if @cretz has anything to add.

cretz

LGTM (though I didn't pour over the details of the latest commits)

siavash added 6 commits July 16, 2023 15:44

feat: support tracing on signals

97561e8

feat: support tracing on signals

6e15b18

feat: support tracing on signals

47d1002

feat: support tracing on updates

acc4296

feat: support tracing on queries

ff21f0a

fix: tests

16fdb02

siavashkavousi requested a review from a team as a code owner July 16, 2023 20:38

cretz reviewed Jul 17, 2023

View reviewed changes

siavash added 4 commits July 17, 2023 17:27

feat: support tracing on queries

74028eb

feat: support tracing on queries

1a44c22

feat: support tracing on queries

a69f5be

feat: support tracing on queries

172cf89

cretz reviewed Jul 25, 2023

View reviewed changes

siavash added 2 commits August 14, 2023 21:42

fix: code review comments

aa24cb9

fix: code review comments

b9744df

refactor: merge latest commits

412552f

siavash added 2 commits September 2, 2023 00:08

refactor: merge latest commits

9f1d16f

refactor: merge latest commits

2e02ef9

siavash added 3 commits September 2, 2023 01:16

fix: test failure

c6dc546

fix: test failure

355c613

fix: test failure

8fb166f

Merge remote-tracking branch 'temporal/master' into tracing-support-i…

1078bf5

…mprovements

Quinn-With-Two-Ns approved these changes Sep 22, 2023

View reviewed changes

cretz approved these changes Sep 25, 2023

View reviewed changes

Quinn-With-Two-Ns merged commit d50d274 into temporalio:master Sep 25, 2023
4 checks passed

siavashkavousi deleted the tracing-support-improvements branch September 30, 2023 07:39

This was referenced Sep 17, 2024

Header support for signals and queries #940

Closed

Use client OpenTracing/OpenTelemetry spans for signal and queries #1306

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tracing support improvements #1819

Tracing support improvements #1819

siavashkavousi commented Jul 16, 2023

CLAassistant commented Jul 16, 2023 •

edited

Loading

cretz left a comment

cretz Jul 17, 2023

siavashkavousi Jul 17, 2023 •

edited

Loading

cretz Jul 24, 2023

siavashkavousi Jul 25, 2023

cretz Jul 25, 2023

cretz Aug 29, 2023

Quinn-With-Two-Ns Aug 29, 2023

cretz Aug 30, 2023

siavashkavousi Aug 31, 2023

Quinn-With-Two-Ns Aug 31, 2023

cretz Jul 17, 2023

siavashkavousi Jul 22, 2023

cretz Jul 17, 2023

siavashkavousi Jul 17, 2023

cretz Jul 17, 2023

siavashkavousi Jul 22, 2023

cretz Jul 24, 2023 •

edited

Loading

cretz Jul 25, 2023

siavashkavousi Aug 14, 2023

Quinn-With-Two-Ns commented Aug 17, 2023

Quinn-With-Two-Ns commented Aug 31, 2023

siavashkavousi commented Sep 1, 2023 •

edited

Loading

siavashkavousi commented Sep 9, 2023

Quinn-With-Two-Ns commented Sep 9, 2023

Quinn-With-Two-Ns commented Sep 15, 2023

siavashkavousi commented Sep 19, 2023

Quinn-With-Two-Ns left a comment

cretz left a comment

	public Tracer.SpanBuilder createWorkflowSignalSpan(
	public Tracer.SpanBuilder createWorkflowHandleSignalSpan(

Tracing support improvements #1819

Tracing support improvements #1819

Conversation

siavashkavousi commented Jul 16, 2023

CLAassistant commented Jul 16, 2023 • edited Loading

cretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siavashkavousi Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quinn-With-Two-Ns commented Aug 17, 2023

Quinn-With-Two-Ns commented Aug 31, 2023

siavashkavousi commented Sep 1, 2023 • edited Loading

siavashkavousi commented Sep 9, 2023

Quinn-With-Two-Ns commented Sep 9, 2023

Quinn-With-Two-Ns commented Sep 15, 2023

siavashkavousi commented Sep 19, 2023

Quinn-With-Two-Ns left a comment

Choose a reason for hiding this comment

cretz left a comment

Choose a reason for hiding this comment

CLAassistant commented Jul 16, 2023 •

edited

Loading

siavashkavousi Jul 17, 2023 •

edited

Loading

cretz Jul 24, 2023 •

edited

Loading

siavashkavousi commented Sep 1, 2023 •

edited

Loading