Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: distributed tracing not working #139

Merged
merged 3 commits into from
Feb 25, 2025
Merged

Conversation

volmedo
Copy link
Member

@volmedo volmedo commented Feb 24, 2025

fixes #134

Context propagation was not properly configured and incoming trace context was not being extracted from requests coming from the gateway.

Interestingly, telemetry was already configured to sample only requests that were sampled in the upstream service, but since context propagation was not working the result was that all requests were being sampled. Fixing context propagation also means that the indexer will only sample requests that are sampled in the gateway.

@volmedo volmedo self-assigned this Feb 24, 2025
Copy link
Member

@frrist frrist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -65,11 +79,41 @@ func InstrumentLambdaHandler(handlerFunc interface{}) interface{} {

return otellambda.InstrumentHandler(
handlerFunc,
otellambda.WithEventToCarrier(jsonEventHeadersToCarrier),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting: https://pkg.go.dev/go.opentelemetry.io/contrib/instrumentation/github.com/aws/aws-lambda-go/otellambda#EventToCarrier

An EventToCarrier function defines how the instrumentation should prepare a TextMapCarrier for the configured propagator to read from. This extra step is necessary because Lambda does not have HTTP headers to read from and instead stores the headers it was invoked with (including TraceID, etc.) as part of the invocation event. If using the AWS XRay tracing then the trace information is instead stored in the Lambda environment.

So this is technically the "fix" for the issue, great find!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah. It all comes from the fact that lambda handlers can have different shapes and receive many event schemas. Honestly, I would've expected otellambda.InstrumentHandler to take care of most of that automatically, at least for the most common events, such as the ones produced by the API Gateway.

Surprisingly enough, the headers are correctly transferred to the request injected into the lambda handler for HTTP requests, so using the standard OpenTelemetry libs for net/http would've been an alternative solution and would've worked out of the box. This approach, however, gives us a bit more lambda-specific information.

Comment on lines 92 to 114
ev := map[string]any{}
err := json.Unmarshal(eventJSON, &ev)
if err != nil {
return propagation.MapCarrier{}
}

// confirm the event has a headers field
if _, ok := ev["headers"]; !ok {
return propagation.MapCarrier{}
}

// confirm the headers field is a map
headers, ok := ev["headers"].(map[string]any)
if !ok {
return propagation.MapCarrier{}
}

headerMap := make(map[string]string, len(headers))
for k, v := range headers {
headerMap[k] = v.(string)
}

return propagation.MapCarrier(headerMap)
Copy link
Member

@frrist frrist Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

We are conforming to this event schema, yeah? https://docs.aws.amazon.com/apigateway/latest/developerguide/http-api-develop-integrations-lambda.html - if so, we could probably condense this a bit to something like:

    var event struct {
        Headers map[string]string `json:"headers"`
    }

    if err := json.Unmarshal(eventJSON, &event); err != nil {
        return propagation.MapCarrier{}
    }

    return propagation.MapCarrier(event.Headers)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absolutely, great point!

@volmedo volmedo merged commit 3f120ef into main Feb 25, 2025
8 checks passed
@volmedo volmedo deleted the vic/fix/distributed-tracing branch February 25, 2025 09:11
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Distributed Tracing Not working
2 participants