feat: send triggering transaction span to application exporter #947

mathnogueira · 2022-07-20T17:36:28Z

This PR makes tracetest send the triggering transaction to the application tracing storage.

Checklist

tested locally
added new dependencies
updated the docs
added a test

mathnogueira · 2022-07-21T18:31:18Z

server/executor/poller_executor.go

+		pe.repeatedTraceSizeCache[trace.ID.String()] = 0
+	}
+
+	if pe.repeatedTraceSizeCache[trace.ID.String()] >= 3 {


This way, we only return true if the number of spans doesn't change after 3 pollings

Decided to remove it from this PR. This solution is too raw to be merged, I'll work on a better approach later.

schoren · 2022-07-22T14:29:23Z

server/executor/poller_executor.go

@@ -73,7 +73,9 @@ func NewPollerExecutor(

 func (pe DefaultPollerExecutor) ExecuteRequest(request *PollingRequest) (bool, model.Run, error) {
 	run := request.run
-	otelTrace, err := pe.traceDB.GetTraceByID(request.ctx, run.TraceID.String())
+	traceId := run.TraceID.String()


nit: go conventions suggest this should be named traceID

schoren · 2022-07-22T14:32:21Z

server/executor/trigger/grpc.go

@@ -49,15 +57,22 @@ func (te *grpcTriggerer) Trigger(_ context.Context, test model.Test, tid trace.T
 	if trigger.GRPC == nil {
 		return response, fmt.Errorf("no settings provided for GRPC triggerer")
 	}
+
+	ctx, span := te.tracer.Start(context.Background(), "Tracetest Trigger")


I think it would be better to have the span and ctx set outside the trigger, so each trigger doesn't have to copy/paste this code

schoren · 2022-07-22T14:33:05Z

server/executor/trigger/http.go

 			otelhttp.WithPropagators(propagators()),
 		),
 	}

+	ctx, span := te.tracer.Start(ctx, "Tracetest Trigger")


schoren · 2022-07-22T14:35:50Z

server/model/json.go

+			Type:    r.TriggerResult.Type,
+			HTTP:    r.TriggerResult.HTTP,
+			GRPC:    r.TriggerResult.GRPC,
+			SpanID:  r.TriggerResult.SpanID.String(),


Why do we need to move the spanID/TraceID from the Run to the trigger?

We either do this or we pass the pointer to the run so we can set the spanID and traceID to the run instance.

…ggering span

schoren · 2022-07-29T14:13:25Z

server/executor/trigger/triggerer.go

@@ -11,7 +11,7 @@ import (
 )

 type Triggerer interface {
-	Trigger(context.Context, model.Test, trace.TraceID, trace.SpanID) (Response, error)
+	Trigger(context.Context, context.Context, model.Test, trace.TraceID, trace.SpanID) (Response, error)


It's really confusing to have 2 contexts here, so it would be great to name these params so we can know what is which

schoren · 2022-07-29T14:14:49Z

server/executor/trigger/http.go


+func (te *httpTriggerer) Trigger(_ context.Context, ctx context.Context, test model.Test, tid trace.TraceID, sid trace.SpanID) (Response, error) {


this looks like the first context should be called ctx and the 2nd triggerSpanCtx or something like that

schoren · 2022-07-29T14:16:00Z

server/executor/trigger/grpc.go


+func (te *grpcTriggerer) Trigger(_ context.Context, ctx context.Context, test model.Test, tid trace.TraceID, sid trace.SpanID) (Response, error) {


this looks like the first context should be called ctx and the 2nd triggerSpanCtx or something like that

schoren · 2022-07-29T14:20:39Z

server/executor/trigger/grpc.go

@@ -93,6 +88,8 @@ func (te *grpcTriggerer) Trigger(_ context.Context, test model.Test, tid trace.T
 		marshaller: marshaler,
 	}

+	propagators().Inject(ctx, NewGRPCHeaderCarrier(&tReq.Metadata))


why do you need a custom carrier? it's probablly easier to do something like:

// see https://pkg.go.dev/go.opentelemetry.io/otel/propagation#MapCarrier propagators().Inject(ctx, propagators.MapCarrier(tReq.Metadata.Map())) // where tReq.Metadata.Map() is a func that returns a map of key->values from the metadata

schoren · 2022-07-29T14:22:03Z

server/executor/trigger/grpc_carrier.go

+	"go.opentelemetry.io/otel/propagation"
+)
+
+type GRPCHeaderCarrier struct {


I would remove this entire implementation and just use https://pkg.go.dev/go.opentelemetry.io/otel/propagation#MapCarrier

schoren · 2022-07-29T14:29:03Z

server/executor/runner.go

@@ -124,13 +127,19 @@ func (r persistentRunner) processExecQueue(job execReq) {
 		panic(err)
 	}

-	response, err := triggerer.Trigger(job.ctx, job.test, job.run.TraceID, job.run.SpanID)
+	triggerSpanCtx, span := r.triggerSpanTracer.Start(context.Background(), "Tracetest trigger")


If you move this to the instrumentedTriggerer.Trigger func, you wouldn't need to use 2 contexts in the Trigger interface. Given that no trigger type is actually using both contexts, I understand that this is passed just because theinstrumentedTriggerer doesn't have the triggerSpanTracer dependency. I'd rather add a dependency to the instrumentedTriggerer than to have 2 contexts on an interface that aren't actually required

Not only that, to do that we either have to:

Add a pointer to the test run to the Trigger interface

Change the trigger response object to have both traceID and spanID

Without either of those, we can't update the run object with the correct traceID and spanID.

* send triggering transaction span to application exporter * fix test * fix deploy * change how we generate the triggering transaction span * make sure all tracing tests include an assertion to check for the triggering span * change how to get a tracer * remove assertion for trigger span * fix http trigger span name * send span and change poller condition * fix test * revert parallel change in test * set spanid and traceid in grpc trigger * remove change in the poller process * make the triggering transaction send the span * fix tests * inject context into grpc call * pr changes * refactor * remove grpc carrier Co-authored-by: Sebastian Choren <[email protected]>

mathnogueira force-pushed the feat/send-span branch from 3a1950b to 70f165c Compare July 20, 2022 21:13

mathnogueira marked this pull request as draft July 20, 2022 21:13

mathnogueira force-pushed the feat/send-span branch from 71b5b1a to 37b8662 Compare July 21, 2022 18:29

mathnogueira commented Jul 21, 2022

View reviewed changes

mathnogueira force-pushed the feat/send-span branch 2 times, most recently from 6a4104b to 7b3c12b Compare July 22, 2022 13:35

mathnogueira marked this pull request as ready for review July 22, 2022 14:10

schoren suggested changes Jul 22, 2022

View reviewed changes

mathnogueira added 14 commits July 26, 2022 13:46

send triggering transaction span to application exporter

95f30f1

fix test

b779971

fix deploy

09cc330

change how we generate the triggering transaction span

57f1852

make sure all tracing tests include an assertion to check for the tri…

9ffee89

…ggering span

change how to get a tracer

ff7d4a1

remove assertion for trigger span

06f128b

fix http trigger span name

abee877

send span and change poller condition

904b103

fix test

1448fb2

revert parallel change in test

e91dc10

set spanid and traceid in grpc trigger

be5a94c

remove change in the poller process

2f3c7b7

make the triggering transaction send the span

33d8e67

mathnogueira force-pushed the feat/send-span branch from 54f7de7 to 33d8e67 Compare July 26, 2022 16:47

mathnogueira added 2 commits July 26, 2022 13:48

fix tests

32442fa

inject context into grpc call

a153181

mathnogueira requested a review from schoren July 26, 2022 20:45

schoren suggested changes Jul 29, 2022

View reviewed changes

mathnogueira and others added 2 commits July 29, 2022 16:20

pr changes

62ade8b

refactor

3c070d9

schoren force-pushed the feat/send-span branch from ee68997 to 3c070d9 Compare July 29, 2022 19:49

remove grpc carrier

0c8d9e3

schoren approved these changes Jul 29, 2022

View reviewed changes

mathnogueira merged commit b7d0deb into main Jul 29, 2022

mathnogueira deleted the feat/send-span branch July 29, 2022 20:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: send triggering transaction span to application exporter #947

feat: send triggering transaction span to application exporter #947

mathnogueira commented Jul 20, 2022

mathnogueira Jul 21, 2022

mathnogueira Jul 22, 2022

schoren Jul 22, 2022

schoren Jul 22, 2022

schoren Jul 22, 2022

schoren Jul 22, 2022

mathnogueira Jul 22, 2022

schoren Jul 29, 2022

schoren Jul 29, 2022

schoren Jul 29, 2022

schoren Jul 29, 2022

schoren Jul 29, 2022

schoren Jul 29, 2022

mathnogueira Jul 29, 2022


		func (te *httpTriggerer) Trigger(_ context.Context, ctx context.Context, test model.Test, tid trace.TraceID, sid trace.SpanID) (Response, error) {


		func (te *grpcTriggerer) Trigger(_ context.Context, ctx context.Context, test model.Test, tid trace.TraceID, sid trace.SpanID) (Response, error) {

feat: send triggering transaction span to application exporter #947

feat: send triggering transaction span to application exporter #947

Conversation

mathnogueira commented Jul 20, 2022

Checklist

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