Skip to content

Feature request: access to the root trace id in Tracer #1070

New issue

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

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

Already on GitHub? Sign in to your account

Closed
misterjoshua opened this issue Aug 22, 2022 · 11 comments · Fixed by #1123
Closed

Feature request: access to the root trace id in Tracer #1070

misterjoshua opened this issue Aug 22, 2022 · 11 comments · Fixed by #1123
Assignees
Labels
completed This item is complete and has been merged/shipped feature-request This item refers to a feature request for an existing or new utility tracer This item relates to the Tracer Utility

Comments

@misterjoshua
Copy link
Contributor

misterjoshua commented Aug 22, 2022

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 misterjoshua added the triage This item has not been triaged by a maintainer, please wait label Aug 22, 2022
@dreamorosi
Copy link
Contributor

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

@ijemmy
Copy link
Contributor

ijemmy commented Aug 23, 2022

@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.

@dreamorosi
Copy link
Contributor

@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.

@saragerion
Copy link
Contributor

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:
#129

@misterjoshua
Copy link
Contributor Author

misterjoshua commented Aug 23, 2022

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.

@dreamorosi
Copy link
Contributor

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?

@sthuber90
Copy link
Contributor

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 😄

@dreamorosi
Copy link
Contributor

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.

@misterjoshua
Copy link
Contributor Author

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.

@github-actions
Copy link
Contributor

⚠️ 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 github-actions bot added the pending-release This item has been merged and will be released soon label Oct 27, 2022
@dreamorosi dreamorosi removed triage This item has not been triaged by a maintainer, please wait pending-release This item has been merged and will be released soon labels Oct 27, 2022
@dreamorosi
Copy link
Contributor

The feature has been released in v1.4.0

@dreamorosi dreamorosi changed the title Feature (tracer): access to the root trace id Feature request: access to the root trace id in Tracer Nov 14, 2022
@dreamorosi dreamorosi added tracer This item relates to the Tracer Utility feature-request This item refers to a feature request for an existing or new utility completed This item is complete and has been merged/shipped labels Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completed This item is complete and has been merged/shipped feature-request This item refers to a feature request for an existing or new utility tracer This item relates to the Tracer Utility
Projects
None yet
5 participants