powertools-lambda-typescript icon indicating copy to clipboard operation
powertools-lambda-typescript copied to clipboard

Feature (tracer): access to the root trace id

Open misterjoshua opened this issue 2 years ago • 7 comments

Description of the feature request

Problem statement When handling application errors, I want to embed the XRay Trace ID in the payload & error message returned by my API so that customers can cite the trace ID in support requests, and so our development team can easily look up the trace from a customer service request via AWS XRay's web console.

Summary of the feature I'd like the tracer to provide the root trace id so that I can access it in my (otherwise) unhandled exceptions handler when I construct a user-facing error message.

Code examples

const rootTraceId = tracer.getRootTraceId();

// Example of returning an error with Express.js
res.status(500).json({
  error: `Internal Error - Please contact support and quote the following id: ${rootTraceId}`,
  // Include the rootTraceId in the response so we can show a "contact support" button that
  // takes the customer to a customer service form with the trace as additional context.
  rootTraceId, 
});

Benefits for you and the wider AWS community Makes it easier for the wider community to expose trace IDs to their customers so that developers can look up the conditions and contexts of the errors reported by their customers.

Describe alternatives you've considered For now, we've implemented this by parsing out the Root= traceid from process.env['_X_AMZN_TRACE_ID'], but our preference would be to depend on Powertools for this.

export function getTraceId(): string | undefined {
  const traceId = process.env['_X_AMZN_TRACE_ID'];
  if (!traceId) return;

  return parseTraceId(traceId);
}

export function parseTraceId(traceId: string): string | undefined {
  if (traceId.includes('Root=')) {
    // Example:
    // Root=1-6303d71b-63c75c564e336c9c2cffc1d5;Parent=58e97d4cc1ed1447;Sampled=1
    const [rootTraceId] = traceId
      .split(';')
      .filter((segment) => segment.startsWith('Root='))
      .map((segment) => {
        const [_, rootTraceId] = segment.split('=');
        return rootTraceId;
      });

    if (rootTraceId) {
      // 1-6303d71b-63c75c564e336c9c2cffc1d5
      return rootTraceId;
    }
  }

  return traceId;
}

Additional context None

Related issues, RFCs

None

misterjoshua avatar Aug 22 '22 20:08 misterjoshua

Hi @misterjoshua thank you for another interesting feature request.

At the moment Tracer doesn't expose any method but I think there's a chance that the aws-xray-sdk does it, I'd have to check. Another datapoint here is that we have a private method that does this in Logger (here). For that utility we use the method to append the xray_trace_id to log entries in some cases.

I wonder if we should instead generalise the logic of this method in the Utility class present in packages/commons, and then use it in Logger & Tracer as private & public respectively.

Would like to hear the take of other maintainers, as well as your opinion on the above, before moving forward

dreamorosi avatar Aug 23 '22 11:08 dreamorosi

@dreamorosi

  1. We probably don't want Logger to depends on aws-xray-sdk. So this option is only suitable if we have the same method in Tracer. But we don't want to have it in common
  2. Moving Logger.getXrayTraceId() logic to common package makes sense to me. From quick look, I think the best place is the class Utility? (in commons/src/Utility.ts). Both Logger and Tracer are using them already.

ijemmy avatar Aug 23 '22 13:08 ijemmy

@dreamorosi

  1. We probably don't want Logger to depends on aws-xray-sdk. So this option is only suitable if we have the same method in Tracer. But we don't want to have it in common

  2. Moving Logger.getXrayTraceId() logic to common package makes sense to me. From quick look, I think the best place is the class Utility? (in commons/src/Utility.ts). Both Logger and Tracer are using them already.

Yes, sorry, point 2 is what I was referring to. I wouldn't want to take a dependency on the X-ray SDK.

I was proposing to either find & use the method from the SDK in the tracer only

OR

Generalize the method into Utility and use it from both.

Either way the bundle would stay more or less the same for all packages involved.

dreamorosi avatar Aug 23 '22 13:08 dreamorosi

I like this idea. I absolutely see scenarios in which correlation IDs generated by AWS including this one might need to be inspected or handled by your application.

Not the same but good for context that there has been some conversation going on in the past about AWS-generated correlation IDs: https://github.com/awslabs/aws-lambda-powertools-typescript/issues/129

saragerion avatar Aug 23 '22 13:08 saragerion

Would like to hear the take of other maintainers, as well as your opinion on the above, before moving forward

I like where this is headed. It makes practical sense to me to move Logger's getXrayTraceId() to commons.

One small point from me: What access modifier are we thinking to use on Utility.getXrayTraceId()? Perhaps it should be protected in Utility and re-exposed with public in only Tracer. I ask because I wonder to what extent exposing logger.getXrayTraceId() or metrics.getXrayTraceId() to customers makes sense.

misterjoshua avatar Aug 23 '22 23:08 misterjoshua

I don't have a strong preference on the access modifier to be used in Utility but for the time being I'd play on the safe side and go with protected as per your suggestion. If in the future we want to expose that API we'll re-evaluate then.

Regarding the subclasses and getXrayTraceId(), at least for now I'm inclined towards:

  • Metrics - does not need it so it doesn't use / expose the method - this could change in the future if an use case comes up
  • Logger - does use it to enrich log entries in some cases, but does not expose it as public API
  • Tracer - does not use it internally, but exposes the method as public API

Please let me know if anyone has any objection on this.

In the meanwhile, another discussion that I'd like to have is around the actual implementation of this method in Utility.

As you'll notice in the current implementation in Logger, the method does not access directly the environment variables but does so via another service, below a copy of the current implementation:

private getXrayTraceId(): string {
  const xRayTraceId = this.getEnvVarsService().getXrayTraceId();

  return xRayTraceId.length > 0 ? xRayTraceId.split(';')[0].replace('Root=', '') : xRayTraceId;
}

This service is EnvironmentVariablesService and generally speaking it's a design that we want to maintain.

The reasons why I bring this up though, are the following:

1. Shared / Duplicate Code

Related issues: #484 (especially this comment & subsequent one), #547

There is a similar implementation of EnvironmentVariablesServices in each one of the three existing utilities, and I expect there will be one in all future utilities that access the environment variables. Each one of these implementations has some common methods and some other unique ones. Given that we are considering moving a method that access the environment variables, I would like to avoid having another similar EnvironmentVariablesServices implementation in Utility and at the same time I would like to avoid having the method read directly from the env vars.

For this reason I think we should have a discussion also on how to implement EnvironmentVariablesServices in a way that minimizes code duplication but also doesn't pollutes the prototype for every utility class too much.

2. Default return value

Related issues: #165

Regardless of the decision on the previous point, the implementation of getXrayTraceId() will have a form of getter method to access an environment variable. This is an opportunity to address that related issue. Note that when I say address I don't necessarily mean change the implementation, this could also be settling on and confirming that we like the current one & closing the issue.

Any thoughts on all these points?

dreamorosi avatar Aug 24 '22 11:08 dreamorosi

I see easy access to the trace ID as helpful. I also agree to the protected point as per your suggestion and agree to de-duplicate code as per #484. Should keep #165 in mind but don't necessarily see it as a blocker or strongly related to the RFC. That being said, if we do it we may as well do it all in one go 😄

sthuber90 avatar Aug 24 '22 13:08 sthuber90

Hi @misterjoshua, @sthuber90, I have opened a PR (#1123) that addresses the changes discussed above.

Would love to have your opinion on the current direction.

dreamorosi avatar Oct 18 '22 09:10 dreamorosi

Would love to have your opinion on the current direction.

Thanks for your work so far. I'll be in a better position to take a look this evening.

misterjoshua avatar Oct 18 '22 17:10 misterjoshua

⚠️ COMMENT VISIBILITY WARNING ⚠️

Comments on closed issues are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so.

github-actions[bot] avatar Oct 27 '22 08:10 github-actions[bot]

The feature has been released in v1.4.0

dreamorosi avatar Oct 27 '22 14:10 dreamorosi