Add trace support into task #55

kaibocai · 2024-01-04T21:47:45Z

This PR add support to support for injection of otel spans into tasks.
Try to resolve #31
protobuf update: microsoft/durabletask-protobuf#21

User can get current span by

span := trace.SpanFromContext(ctx.Context())

or create child span from current span

_, childSpan := tracer.Start(ctx.Context(), "activity-subwork")

The output trace looks like

kaibocai · 2024-01-04T21:50:06Z

tagging @ItalyPaleAle for review.

ItalyPaleAle · 2024-01-04T22:25:12Z

internal/protos/orchestrator_service.pb.go

@@ -3,7 +3,7 @@

 // Code generated by protoc-gen-go. DO NOT EDIT.
 // versions:
-// 	protoc-gen-go v1.31.0
+// 	protoc-gen-go v1.28.1


Note protoc-gen-go was downgraded. Not sure if it matters

Thanks for catching this. different versions of protoc-gen-go are used by developers to generate protobuf code, I think as long as tests run well, it should be fine. Will update my local protoc-gen-go version though.

cgillum · 2024-01-10T00:54:59Z

task/activity.go

@@ -52,6 +54,10 @@ type activityContext struct {
 type Activity func(ctx ActivityContext) (any, error)

 func newTaskActivityContext(ctx context.Context, taskID int32, ts *protos.TaskScheduledEvent) *activityContext {
+	ctx, err := helpers.ContextFromTraceContext(ctx, ts.ParentTraceContext)


I would expect that the ctx argument already has the correct trace context, being set much earlier than this.

Yes, for examples running not through the grpc we did have the trace context, but if we run it through grpc we are missing it.

We can actually set the context in processWorkItem itself:

durabletask-go/client/worker_grpc.go

Line 92 in fc57567

func (c *TaskHubGrpcClient) processActivityWorkItem(

cgillum · 2024-01-10T00:57:14Z

client/worker_grpc.go

@@ -94,7 +94,7 @@ func (c *TaskHubGrpcClient) processActivityWorkItem(
 	executor backend.Executor,
 	req *protos.ActivityRequest,
 ) {
-	var tc *protos.TraceContext = nil // TODO: How to populate trace context?
+	var tc *protos.TraceContext = req.ParentTraceContext


I need more help understanding why we need to pass ParentTraceContext on this request object instead of just getting it directly from the gRPC context. Is it because work items are received on a stream and gRPC doesn't automatically populate any distributed trace context?

I think

the grpc context cannot directly used to pass through key-value pair (span is usually stored as key-val pair in context in go), instead it needs a bit more work https://www.hward.com/golang-grpc-context-client-server/.

we obtain the stream at

durabletask-go/client/worker_grpc.go

Line 26 in fc57567

stream, err := c.client.GetWorkItems(ctx, &req)

, at this point we don't have any trace context set up in the context, the context basically is an empty context. The stream should not have any trace info therefore.

I tried a local run to assert this
This is the context we build up on grpc server side

This is the context at

durabletask-go/client/worker_grpc.go

Line 93 in fc57567

ctx context.Context,

Copying from here: #31 (comment)

Agree @kaibocai. I looked into it as well. We can easily propagate context for a normal grpc request(non-streaming) using otel interceptors(e.g. here) but for streaming requests, context is propagated at the start only i.e. when the stream is created. Once the stream creation is done, then since no context is passed during stream.send() method, and because we can't update stream context, the newer context(updated after stream creation) is never passed to client/server.

Here in DTF-Go as well at the time of stream creation, there is nothing in context(since no orchestration/acitivity are created yet). We have the updated context(with all span info) when activity/orchestration workitem needs to be sent to the app but this context is not passed while sending the workItem. So best way to do it is to encode trace context in the message itself.

shivamkm07

Should we add ParentTraceContext in OrchestratorRequest as well as well, similar to activityRequest? It would enable users to create spans/use trace informations in orchestrations as well. thoughts @kaibocai @cgillum

durabletask-go/internal/protos/orchestrator_service.pb.go

Line 2573 in fc57567

type OrchestratorRequest struct {

shivamkm07 · 2024-01-11T05:00:51Z

backend/activity.go

@@ -68,6 +68,8 @@ func (p *activityProcessor) ProcessWorkItem(ctx context.Context, wi WorkItem) er
 		}()
 	}

+	// set the parent trace context to be the newly created activity span
+	ts.ParentTraceContext = helpers.TraceContextFromSpan(span)


No need to reset the ParentTraceContext here as the newly created context above already has the span information. It can be extracted in the ExecuteActivity itself.

I think I still need to set the ParentTraceContext as I need to populate it here

durabletask-go/backend/executor.go

Line 151 in d11688c

ParentTraceContext: task.ParentTraceContext,

We can actually get span from context itself: https://pkg.go.dev/go.opentelemetry.io/otel/trace#SpanFromContext and then get traceContext from span, but i guess it's easier updating the event itself.

My issue was updating the event was that it should't cause any issue in case of errors/retries(like recursively adding a new span under activity span), but it doesn't seem anything like that happening for now, so no issues.

shivamkm07 · 2024-01-11T05:07:10Z

task/activity.go

@@ -52,6 +54,10 @@ type activityContext struct {
 type Activity func(ctx ActivityContext) (any, error)

 func newTaskActivityContext(ctx context.Context, taskID int32, ts *protos.TaskScheduledEvent) *activityContext {
+	ctx, err := helpers.ContextFromTraceContext(ctx, ts.ParentTraceContext)


We can actually set the context in processWorkItem itself:

durabletask-go/client/worker_grpc.go

Line 92 in fc57567

func (c *TaskHubGrpcClient) processActivityWorkItem(

shivamkm07 · 2024-01-11T05:10:06Z

tests/orchestrations_test.go

@@ -207,6 +210,56 @@ func Test_SingleActivity(t *testing.T) {
 	)
 }

+func Test_SingleActivity_TaskSpan(t *testing.T) {


There should be a test added in https://github.com/microsoft/durabletask-go/blob/main/tests/grpc/grpc_test.go as well to validate we are correctly passing the context information over grpc stream.

kaibocai · 2024-01-11T20:04:07Z

Should we add ParentTraceContext in OrchestratorRequest as well as well, similar to activityRequest? It would enable users to create spans/use trace informations in orchestrations as well.

My thought is that in orchestration users are simply calling APIs we provided to do their orchestration, it seems to me that there is not much need to create child spans inside orchestration. All the operations will be simple and the span we generated here should be enough.

durabletask-go/backend/executor.go

Line 311 in fc57567

    
           ctx, span := helpers.StartNewCreateOrchestrationSpan(ctx, req.Name, req.Version.GetValue(), instanceID)

shivamkm07 · 2024-01-12T05:13:30Z

main.go

@@ -23,6 +28,17 @@ func main() {
 	// Parse command-line arguments
 	flag.Parse()

+	// Tracing can be configured independently of the orchestration code.
+	tp, err := ConfigureZipkinTracing()


Should this be under some flag like use-zipkin or some other name? In case user is using any collector other than zipkin?

This change was the wrong commit, I will revert it. Thanks for catching it. This PR will not include changes for setting up the exporter, it only includes changes for injecting trace info to activity.

kaibocai added 3 commits January 4, 2024 13:31

add trace support into task

33790bd

clean up

9fd912b

simplify creating span context

d11688c

kaibocai requested a review from cgillum January 4, 2024 21:47

ItalyPaleAle approved these changes Jan 4, 2024

View reviewed changes

kaibocai marked this pull request as draft January 5, 2024 23:31

init exporter on server side

3bbf9d1

cgillum reviewed Jan 10, 2024

View reviewed changes

shivamkm07 reviewed Jan 11, 2024

View reviewed changes

add grpc test case

a5b3495

fix test name

7a608b7

shivamkm07 reviewed Jan 12, 2024

View reviewed changes

remove exporter

ab03cd0

kaibocai marked this pull request as ready for review January 12, 2024 21:06

shivamkm07 approved these changes Jan 15, 2024

View reviewed changes

Merge branch 'main' into kaibocai/otel-task

14a9bda

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add trace support into task #55

Add trace support into task #55

kaibocai commented Jan 4, 2024 •

edited

Loading

kaibocai commented Jan 4, 2024

ItalyPaleAle Jan 4, 2024

kaibocai Jan 4, 2024

cgillum Jan 10, 2024

kaibocai Jan 10, 2024

shivamkm07 Jan 11, 2024

cgillum Jan 10, 2024

kaibocai Jan 10, 2024

shivamkm07 Jan 11, 2024

shivamkm07 left a comment

shivamkm07 Jan 11, 2024

kaibocai Jan 11, 2024

shivamkm07 Jan 12, 2024

shivamkm07 Jan 11, 2024

shivamkm07 Jan 11, 2024

kaibocai commented Jan 11, 2024

shivamkm07 Jan 12, 2024

kaibocai Jan 12, 2024

Add trace support into task #55

Are you sure you want to change the base?

Add trace support into task #55

Conversation

kaibocai commented Jan 4, 2024 • edited Loading

kaibocai commented Jan 4, 2024

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

shivamkm07 left a 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

kaibocai commented Jan 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaibocai commented Jan 4, 2024 •

edited

Loading