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

feat(logging): Add correlation_id support #321

Merged
merged 8 commits into from
Mar 10, 2021

Conversation

michaelbrewer
Copy link
Contributor

@michaelbrewer michaelbrewer commented Mar 9, 2021

Issue #, if available:

#9

Description of changes:

Add new method set_correlation_id to allow for customers to set correlation_id in the logger

Example API Gateway Proxy Event

{
    "requestContext":{
        "requestId":"13B398ED-FABA-4D50-A4A1-6CD39BDA85E5"
    }
}

Usage via logger.set_correlation_id:

from aws_lambda_powertools import Logger
from aws_lambda_powertools.utilities.data_classes import APIGatewayProxyEvent

logger = Logger()

def handler(event, context):
    event = APIGatewayProxyEvent(event)
    logger.set_correlation_id(event.request_context.request_id)
    logger.info("Collecting payment")
    ...

Via correlation_id_path

from aws_lambda_powertools import Logger
from aws_lambda_powertools.logging import correlation_paths

logger = Logger()

@logger.inject_lambda_context(correlation_id_path=correlation_paths.API_GATEWAY_REST)
def handler(event, context):
      logger.info("Foo")

Cloudwatch log

{
    "level":"INFO",
    "location":"handler:450",
    "message":"Collecting payment",
    "timestamp":"2021-03-09 15:12:32,652",
    "service":"81shAbHB2IPQTex",
    "sampling_rate":0.0,
    "correlation_id":"13B398ED-FABA-4D50-A4A1-6CD39BDA85E5"
}

Checklist

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

Michael Brewer added 2 commits March 9, 2021 15:10
Prototype idea for correlation_id support in logger
@heitorlessa
Copy link
Contributor

@michaelbrewer could you push an empty commit to test the label one more time, plz?

@michaelbrewer michaelbrewer marked this pull request as ready for review March 10, 2021 06:48
@michaelbrewer michaelbrewer changed the title feat(logging): inject correlation_id feat(logging): Add correlation_id support Mar 10, 2021
@heitorlessa
Copy link
Contributor

heitorlessa commented Mar 10, 2021

That's great, thank you! While we think through the other RFC to allow a more cohesive correlation ID across utilities, may I ask two enhancements where one is optional?

  1. A pre-defined set of correlation ID keys based on Event Sources we know? e.g. API Gateway, ALB, and EventBridge to start with validation envelopes example
  2. [Optional] Allow a JMESPath value to cover use cases where correlation_id might be deeper in the stack as well as combine/filter/slice values

I say optional because dictionary lookup will always be faster than JMESPath, so we'd want to do the former first and have JMESPath as a fallback mechanism instead of doing recursion ourselves.

@heitorlessa
Copy link
Contributor

Scratch that.. too early in the day for me. It's indeed better to handle that in the Uber Supreme Leader Handler, where we could have a Config available to all utilities, where Logger could use, Metrics could use to set correlation ID as metadata, Tracer as annotation, etc.

It kinda sucks though having to ask customers to add an additional line to set correlation ID when they likely already have inject_lambda_context - What do you think of also adding an extra parameter in that decorator?

@codecov-io
Copy link

Codecov Report

Merging #321 (dce2010) into develop (a697436) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #321   +/-   ##
========================================
  Coverage    99.88%   99.88%           
========================================
  Files           91       91           
  Lines         3347     3352    +5     
  Branches       164      165    +1     
========================================
+ Hits          3343     3348    +5     
  Misses           2        2           
  Partials         2        2           
Impacted Files Coverage Δ
aws_lambda_powertools/logging/logger.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a697436...dce2010. Read the comment docs.

docs/core/logger.md Outdated Show resolved Hide resolved
Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

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

One tiny change to improve SEO on docs, and we're good to merge 🎉 !

I'll write another PR to include the new correlation_path in the docs too

@heitorlessa
Copy link
Contributor

@pankajagrawal16 FYI - We're adding the first iteration of setting correlation ID. This is before we work on an uber duper decorator to rule them all with a single config to prevent the confusion of many decorators while also allowing customers to create their own.

The latter doesn't necessarily translates to Java, but the correlation ID is a good one we want to have in the core utilities

Michael Brewer and others added 2 commits March 9, 2021 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants