chore(spanner): Add changes in Spanner executor for testing end to end tracing #10450
Conversation
…into systest-otel-context
…into systest-changes
…into e2e-trace-optin
…into systest-changes
…into systest-changes
spanner/test/cloudexecutor/executor/internal/inputstream/handler.go
Outdated
Show resolved
Hide resolved
spanner/test/cloudexecutor/executor/internal/inputstream/handler.go
Outdated
Show resolved
Hide resolved
…into systest-changes
…into systest-changes
…into systest-changes
…into systest-changes
…into systest-changes
| @@ -50,7 +50,7 @@ func (h *StartBatchTxnHandler) ExecuteAction(ctx context.Context) error { | |||
| return h.OutcomeSender.FinishWithError(spanner.ToSpannerError(status.Error(codes.InvalidArgument, "database path must be set for this action"))) | |||
| } | |||
|
|
|||
| client, err := spanner.NewClient(ctx, h.FlowContext.Database, h.Options...) | |||
| client, err := spanner.NewClientWithConfig(ctx, h.FlowContext.Database, spanner.ClientConfig{SessionPoolConfig: spanner.DefaultSessionPoolConfig, DisableRouteToLeader: false, EnableServerSideTracing: true}, h.Options...) | |||
There was a problem hiding this comment.
Do we need to set SessionPoolConfig and DisableRouteToLeader here? They should already have their default values right?
There was a problem hiding this comment.
I think we will need to set it explicitly. Even spanner.NewClient method sets the default values explicitly in its implementation.
| @@ -331,6 +335,12 @@ type ClientConfig struct { | |||
| DirectedReadOptions *sppb.DirectedReadOptions | |||
|
|
|||
| OpenTelemetryMeterProvider metric.MeterProvider | |||
|
|
|||
| // EnableServerSideTracing indicates whether server side tracing is enabled or not. | |||
There was a problem hiding this comment.
Lets add same comment as that in java PR
There was a problem hiding this comment.
Makes sense. Updated the opt-in header PR(#10241) as well.
There was a problem hiding this comment.
Why do we have same changes in 2 different PRs. If https://github.com/googleapis/google-cloud-go/pull/10241/files is valid, then remove these changes from here and lets have the executor changes alone.
There was a problem hiding this comment.
This PR is created on top of the changes of #10241. Executor changes also includes the usage of EnableServerSideTracing flag which is in #10241. That's why copied the changes from that PR.
Considering this PR will be submitted after #10241 is submitted, When we submit this PR, can't we take a pull from main branch?
spanner/test/cloudexecutor/executor/executor_proxy_server_impl.go
Outdated
Show resolved
Hide resolved
| @@ -142,6 +209,48 @@ func (h *CloudStreamHandler) startHandlingRequest(ctx context.Context, req *exec | |||
| } | |||
| } | |||
|
|
|||
| // verifyCloudTraceExportedTraces fetches the traces exported from client application using | |||
| // Cloud Trace API to cross verify if end to end tracing is working or not. | |||
| func (h *CloudStreamHandler) verifyCloudTraceExportedTraces(ctx context.Context, traceId string) error { | |||
There was a problem hiding this comment.
looks like this method is getting called only once after all the actions complete. If that is right, and TraceClient is getting used only at this place then there is no need to create it way before and store it in the execution flow context. You can create the trace client here and close it once the verification completes.
WDYT?
Is the traceclient used somewhere else?
There was a problem hiding this comment.
No, trace client is only used at the end and doesn't need to be created at the start. thanks for pointing out. Made the change.
…into systest-changes
…into systest-changes
This PR adds changes in spanner executor code required for testing end to end tracing feature.