Propogate Baggage in OTEL tracing interceptor

[jvonfricken]

What was changed

This PR updates the OTEL tracing interceptor implementation to allow OTEL Baggage to be propagated.

Why?

This seems to be an omission from the existing OTEL tracing interceptor implementation, and should be included so that Temporal doesn't break baggage propagation. For my particular use-case, this allows for spans to add baggage as attributes in the SpanStarter func.

Checklist

1. Closes

2. How was this tested:
   The existing unit tests were used to ensure this didn't break existing functionality. Though, there doesn't seem to be a way to include context (for the purpose of including baggage) in the emulated test environment. As such, I'm looking for guidance on how to best test this MR.

3. Any docs updates needed?

No

[CLAassistant]

All committers have signed the CLA.

[cretz]

This seems to be an omission from the existing OTEL tracing interceptor implementation

Yes, with OpenTracing I think this is done automatically so wasn't considered when originally writing

For my particular use-case, this allows for spans to add baggage as attributes in the SpanStarter func.

I wonder if we should make this opt-in. Can you see any downsides to turning on baggage propagation/serialization by default for users that are already using this with regular span propagation/serialization? I assume compatible in both directions.

As such, I'm looking for guidance on how to best test this MR.

You may have to add something below AssertSpanPropagation at https://github.com/temporalio/sdk-go/blob/master/contrib/opentelemetry/tracing_interceptor_test.go to confirm that baggage was propagated throughout or write a whole new test doing similar to what https://github.com/temporalio/sdk-go/blob/master/internal/interceptortest/tracing.go does but checking that baggage is present at each step (e.g. even accessible in activities and workflows).

[jvonfricken]

@cretz

You may have to add something below AssertSpanPropagation at https://github.com/temporalio/sdk-go/blob/master/contrib/opentelemetry/tracing_interceptor_test.go to confirm that baggage was propagated throughout or write a whole new test doing similar to what https://github.com/temporalio/sdk-go/blob/master/internal/interceptortest/tracing.go does but checking that baggage is present at each step (e.g. even accessible in activities and workflows).

Yeah, that makes complete sense. However, in order to do that validation on the intermediate steps, I'd need to get the workflow to run with some baggage added to the context. But as far as I can tell, there's no way to execute a workflow in the emulated environment with a context? With a real workflow execution, I can do the following:

we, err := t.temporalClient.ExecuteWorkflow(
	ctx,
	client.StartWorkflowOptions{
		ID:        "someid",
	},
	ExampleWorkflow,
	ExampleWorkflowParams{},
)

I can simply pass a context.Context that has baggage.Baggage added. However, executing a workflow with the emulated environment looks like this:

env.ExecuteWorkflow(ExampleWorkflow, ExampleWorkflowParams{})

There's no way for me to execute the workflow with a context that has baggage.Baggage embedded, and I can't seem to find any other way of writing to the context otherwise.

Do you have any thoughts about how I could execute a workflow with env.ExecuteWorkflow that has a context with baggage.Baggage added?

[cretz]

Do you have any thoughts about how I could execute a workflow with env.ExecuteWorkflow that has a context with baggage.Baggage added?

I am afraid not, these tests are meant to check the span creation I guess, not the propagation. So let's update the integration tests instead. See TestOpenTelemetryTracing in test/integration_test.go, you may be able to inject baggage there and work with that test. That uses a real client/server.

[jvonfricken]

Ah excellent. I'll look at TestOpenTelemetryTracing

Thanks so much!

I wonder if we should make this opt-in. Can you see any downsides to turning on baggage propagation/serialization by default for users that are already using this with regular span propagation/serialization? I assume compatible in both directions.

Given the existing default NewCompositeTextMapPropagator:

sdk-go/contrib/opentelemetry/tracing_interceptor.go

Lines 42 to 45 in 83199af

	var DefaultTextMapPropagator = propagation.NewCompositeTextMapPropagator(
	propagation.TraceContext{},
	propagation.Baggage{},
	)

I'd say its reasonable reasonable to expect that baggage is already propagating. In other OTEL libraries I've seen, they also propagate baggage by default. Overall, I don't see any issues with leaving the baggage propagation as the default behavior.

[cretz]

Thanks for the test, starting CI now. Added request for opt-out behavior just as a safety measure since we are technically altering existing behavior (even if the existing behavior wasn't the best).

[cretz]

Sorry to sound tedious, but can we add an option to at least opt-out of baggage (i.e. TracerOptions.DisableBaggage)? That way if someone comes and says "hey, why did you start sending my baggage across and change behavior?" we can at least tell them they can disable it?

[jvonfricken]

Absolutely! I'll make that change today

[jvonfricken]

Okay, I added that opt-in option. I don't completely understand under which circumstances the OTEL span lifecycle methods (SpanFromContext, MarshalSpan, UnmarshalSpan, ContextWithSpan, StartSpan) get called by the tracing library, so I conditionalized baggage on all of them. Let me know if you think any of those changes are unnecessary.

I also added an additional test for the opt-out behavior.

[cretz]

I think adding the condition around all of them is the right approach. Approved, looks great! I would like one other developer's review (@Quinn-With-Two-Ns) before merging just in case he has any concerns here.

[Quinn-With-Two-Ns]

LGTM, we (Temporal) should verify if we need to any work in other SDKs to support this.