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

feat(tracer): Support for external observability providers - Tracer

Open roger-zhangg opened this issue 2 years ago • 6 comments

Issue number: 2030

Summary

Support provider in tracer

Changes

Modify the provider with more genreral tracer feature, provide a new x-ray provider.

User experience

This change doesn't affect users who just use default x-ray provider. This change allows users to bring their own tracer provider to use other solution provider other than X-ray

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Is this a breaking change?

RFC issue number:

Checklist:

  • [ ] Migration process documented
  • [ ] Implement warnings (if it can live side by side)

Acknowledgment

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

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

roger-zhangg avatar May 30 '23 20:05 roger-zhangg

Drafted the first PR for tracer provider.

  • Provided a more general Tracer Provider interface.
  • Drafted a x-ray provider I wasn't included in the initial discussions in this projects so I might have missed something.

Questions:

  • Decide how to refactor BaseSegment Class
  • Check on current x-ray provider's design.(Doesn't support add_span, end_span)
  • In the RFC there's a capture_method function included in the provider. But currently this function is in Tracer class. Do we need to move it into our provider?

roger-zhangg avatar May 30 '23 20:05 roger-zhangg

Codecov Report

Attention: Patch coverage is 75.22936% with 27 lines in your changes missing coverage. Please review.

Project coverage is 95.71%. Comparing base (e14e768) to head (7efe7e4). Report is 806 commits behind head on develop.

Files Patch % Lines
...da_powertools/tracing/provider/xray/xray_tracer.py 69.23% 17 Missing and 3 partials :warning:
aws_lambda_powertools/tracing/tracer.py 69.56% 7 Missing :warning:
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2342      +/-   ##
===========================================
- Coverage    96.38%   95.71%   -0.67%     
===========================================
  Files          214      218       +4     
  Lines        10030    10488     +458     
  Branches      1846     1945      +99     
===========================================
+ Hits          9667    10039     +372     
- Misses         259      332      +73     
- Partials       104      117      +13     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Jan 31 '24 21:01 codecov-commenter

Hey @rubenfonseca! Could you please review this PR? As we discussed offline, here are some key points to consider during the review:

1 - DD and OTEL are just placeholders/examples in the code and will be removed before merging this PR. 2 - We've updated naming conventions to use more conventional names in the methods. 3 - Regarding the example, would it be better to create a new one instead of modifying the existing example?

Thanks for your attention to these points.

leandrodamascena avatar Apr 09 '24 09:04 leandrodamascena

@rubenfonseca Thanks for the super detailed feedback. I've resolved all the comments now. Please help with a final review, thanks.

roger-zhangg avatar Apr 12 '24 22:04 roger-zhangg

We can consider this PR completed, but we will not merge it now. We'll start conversations about Powertools V3 and add this refactor when we're ready for it:

1 - We will deprecate the BaseSegment in Powertools v3. 2 - This is a potential breaking change for customers who are accessing the XRay methods directly. 3 - We can take this opportunity to standardize method names like trace instead of subsegment in our documentation and ease for customers adopt new Observability providers.

I'm approving this PR and changing it to Draft state just so I don't mess up our stats.

leandrodamascena avatar Apr 17 '24 16:04 leandrodamascena

Hi @roger-zhangg! I'm closing this PR in favor of #4902! It was really hard to fix the entire merge conflict here, so I've created a new one.

Thank you for all the effort you've put in here, and I'll give you credit for this work when I release v3.

leandrodamascena avatar Aug 06 '24 23:08 leandrodamascena