Skip to content
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

docs(tracer): Allow to reuse Tracer across your code #767

Closed
wants to merge 4 commits into from

Conversation

dreamorosi
Copy link
Contributor

@dreamorosi dreamorosi commented Apr 14, 2022

Description of your changes

When using Tracer customers might want to trace parts of code that are distributed across files/modules. At the moment, in order to maintain the correct relationship between segments, customers would have had to either pass the Tracer instance or a parent segment around, which is tedious and leads to tighter coupling.

This PR makes Tracer reusable by returning the a previously initialised instance, if one exists. With this change customers can:

// index.ts
import { Tracer } from '@aws-lambda-powertools/tracer';
import collectPayment from './payment'; 

const tracer = new Tracer();

export const handler = async (event: unknown) => {
    const segment = tracer.getSegment(); // This is the facade segment (the one that is created by AWS Lambda)
    // Create subsegment for the function & set it as active
    const subsegment = segment.addNewSubsegment(`## ${process.env._HANDLER}`);
    tracer.setSegment(subsegment);

    await collectPayment(event.chargeId);

    ....
};

and then do:

// payment.ts
import { Tracer } from '@aws-lambda-powertools/tracer';

const tracer = new Tracer();

const collectPayment = async (chargeId: string) => {
  // process charge
 tracer.putAnnotation('chargeId', chargeId); // This annotation will be done on the `## index.handler` segment 
};

export default collectPayment;

or:

// payment.ts
import { Tracer } from '@aws-lambda-powertools/tracer';

const tracer = new Tracer();

const collectPayment = async (chargeId: string) => {
  const mainSegment = tracer.getSegment();
  const subsegment = mainSegment.addNewSubsegment('### collectPayment');
  tracer.setSegment(subsegment);

  // This annotation will be done on the `### collectPayment` segment 
  tracer.putAnnotation('chargeId', chargeId);

   // Process charge

  subsegment.close();
  tracer.setSegment(mainSegment);
};

export default collectPayment;

How to verify this change

See existing & new test cases passing.

See screenshot of segments generated by second example above:
image

See video of how the docs look after the changes:

Screen.Recording.2022-04-14.at.13.03.20.mp4

Related issues, RFCs

#275

PR status

Is this ready for review?: YES
Is it a breaking change?: NO

Checklist

  • My changes meet the tenets criteria
  • I have performed a self-review of my own code
  • I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • The code coverage hasn't decreased
  • I have added tests that prove my change is effective and works
  • New and existing unit tests pass locally and in Github Actions
  • The PR title follows the conventional commit semantics

Breaking change checklist

N/A


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@dreamorosi dreamorosi added enhancement tracer This item relates to the Tracer Utility labels Apr 14, 2022
@dreamorosi dreamorosi added this to the production-ready-release milestone Apr 14, 2022
@dreamorosi dreamorosi self-assigned this Apr 14, 2022
@dreamorosi dreamorosi linked an issue Apr 14, 2022 that may be closed by this pull request
4 tasks
@humanzz
Copy link

humanzz commented Apr 16, 2022

I wonder if this singleton pattern should be done for all 3 utilities; tracer, logger and metrics. In larger projects, with many files and modules, this would be really useful I believe.

@dreamorosi
Copy link
Contributor Author

I wonder if this singleton pattern should be done for all 3 utilities; tracer, logger and metrics. In larger projects, with many files and modules, this would be really useful I believe.

It's something we can consider. Could you please open an issue requesting the feature? We'll then prioritize accordingly.

@humanzz
Copy link

humanzz commented Apr 17, 2022

I wonder if this singleton pattern should be done for all 3 utilities; tracer, logger and metrics. In larger projects, with many files and modules, this would be really useful I believe.

It's something we can consider. Could you please open an issue requesting the feature? We'll then prioritize accordingly.

@dreamorosi here it is #777

@dreamorosi dreamorosi added documentation Improvements or additions to documentation and removed enhancement labels Apr 21, 2022
@dreamorosi
Copy link
Contributor Author

dreamorosi commented Apr 21, 2022

Following a discussion with the other maintainers we have decided to scrap this feature in order to avoid forcing a pattern on customers and also in light of the behaviour detailed here.

In Javascript a common pattern is to define a common module that instantiates shared classes and export them.

For example, customers could create a module (even in a Lambda Layer) called powertools.ts, that imports the utilities and instantiates the modules with common configurations:

import { Logger } from '@aws-lambda-powertools/logger';
import { Metrics } from '@aws-lambda-powertools/metrics';
import { Tracer } from '@aws-lambda-powertools/tracer';

const awsLambdaPowertoolsVersion = '0.6.0';

const defaultValues = {
  awsAccountId: process.env.AWS_ACCOUNT_ID || 'N/A',
  environment:  process.env.ENVIRONMENT || 'N/A'
};

const logger = new Logger({
  persistentLogAttributes: {
    ...defaultValues,
    logger: {
      name: '@aws-lambda-powertools/logger',
      version: awsLambdaPowertoolsVersion,
    }
  },
});

const metrics = new Metrics({
  defaultDimensions: defaultValues
});

const tracer = new Tracer();

export {
  logger,
  metrics,
  tracer
};

This module can then be imported and used in other places and files:

// payment.ts
import { tracer, logger } from './powertools.ts';

const collectPayment = async (chargeId: string) => {
  const mainSegment = tracer.getSegment();
  const subsegment = mainSegment.addNewSubsegment('### collectPayment');
  tracer.setSegment(subsegment);

  // This annotation will be done on the `### collectPayment` segment 
  tracer.putAnnotation('chargeId', chargeId);

   // Process charge

   // This log will have the same config & persistent keys as the others
   logger.info('charge processed', { chargeId: chargeId });

  subsegment.close();
  tracer.setSegment(mainSegment);
};

export default collectPayment;

and

import middy from '@middy/core';
import { captureLambdaHandler } from 'from '@aws-lambda-powertools/tracer';
import { injectLambdaContext } from 'from '@aws-lambda-powertools/logger';
import { tracer, logger } from './powertools.ts';
import collectPayment from './payment.ts';

export const handler = middy(async (event: unknown) => {

    await collectPayment(event.chargeId);

    ....

    logger.info('Done');
}).use(captureLambdaHandler(tracer)).use(injectLambdaContext(logger));

This PR will now focus on documenting this pattern in the documentation with the aim of providing prescriptive guidance about this use case.

@dreamorosi dreamorosi changed the title feat(tracer): Allow to reuse Tracer across your code docs(tracer): Allow to reuse Tracer across your code Apr 21, 2022
@dreamorosi dreamorosi marked this pull request as draft April 21, 2022 10:07
@ijemmy
Copy link
Contributor

ijemmy commented Apr 21, 2022

Following a discussion with the other maintainers we have decided to scrap this feature in order to avoid forcing a pattern on customers and also in light of the behaviour detailed here.
....

This PR will now focus on documenting this pattern in the documentation with the aim of providing prescriptive guidance about this use case.

After reading #777 , two options came to my mind:

  1. Should we also update the examples/cdk to use this pattern?
  2. Should we have a "Recommended practices" section with this topic? This is applicable for all utilities. Then we could have a link for each utility doc to this page.

@dreamorosi
Copy link
Contributor Author

Following a discussion with the other maintainers we have decided to scrap this feature in order to avoid forcing a pattern on customers and also in light of the behaviour detailed here.
....
This PR will now focus on documenting this pattern in the documentation with the aim of providing prescriptive guidance about this use case.

After reading #777 , two options came to my mind:

  1. Should we also update the examples/cdk to use this pattern?
  2. Should we have a "Recommended practices" section with this topic? This is applicable for all utilities. Then we could have a link for each utility doc to this page.
  1. Yes, I think we should but I would wait till at least docs(examples): add example for AWS SAM #674 is merged. At that point we'll have to refactor the CDK examples anyways so it'll be a good moment.
  2. I like the idea and I think we should open a separate issue referencing Documentation: reuse across code files/modules #777 and this PR to track it. I don't think it's critical for our current milestone so if we all agree I would propose to:
  • scrap and close this PR
  • close Feature request: tracer feature improvements #275 since this is not a feature we'll implement (it's a pattern) and all others have been already merged
  • keep the soon-to-be-opened issue about documenting this in a "Recommended practices" section & prioritise it after RC/GA.

@dreamorosi
Copy link
Contributor Author

We are going to be closing this PR as detailed here.

We will, after RC/GA release, work on a "Recommended/Best Practices" section in the docs that will showcase this pattern for all modules. We will be tracking this effort in #777.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation tracer This item relates to the Tracer Utility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: tracer feature improvements
3 participants