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

RFC: Update, view and remove the correlation id #25

Closed
Nr18 opened this issue Jul 10, 2021 · 4 comments
Closed

RFC: Update, view and remove the correlation id #25

Nr18 opened this issue Jul 10, 2021 · 4 comments
Labels
Logger Logger utility Proposed Community submited RFC

Comments

@Nr18
Copy link

Nr18 commented Jul 10, 2021

Key information

Summary

When using the correlation feature in conjunction with the SQS you may want to update the correlation id for each message in the incoming batch. This allows you to correlate log lines that are being processed in a batch with the lines in an other log group using the CloudWatch Logs Insights.

Further more next to lines that are related to the message there are also lines that are related to the processing of the batch itself. So an easy way to remove the correlation id would be beneficial to have.

Optionally, it might be useful to fetch the current correlation id for displaying purposes.

Motivation

The following code "works" but it breaks the typing as logger.set_correlation_id expects a str and not a Optional[str]:

from aws_lambda_powertools import Logger
from aws_lambda_powertools.utilities.typing import LambdaContext
from aws_lambda_powertools.utilities.batch import sqs_batch_processor
from aws_lambda_powertools.utilities.data_classes.sqs_event import SQSRecord
logger = Logger(service="callback")


def record_handler(record: dict) -> None:
    record = SQSRecord(record)
    # Get the correlation id from the message attributes
    correlation_id = record.message_attributes["correlation_id"].string_value
    logger.set_correlation_id(correlation_id)
    logger.info(f"Processing message with {correlation_id} as correlation_id")


@sqs_batch_processor(record_handler=record_handler)
def lambda_handler(event: dict, context: LambdaContext) -> None:
    logger.set_correlation_id(None)
    logger.info(f"Received a SQSEvent with {len(list(event.records))} records")

If you want to display the current correlation id you need to track it in your own logic, for example:

from aws_lambda_powertools import Logger
from aws_lambda_powertools.logging import correlation_paths
from aws_lambda_powertools.utilities.typing import LambdaContext
from aws_lambda_powertools.utilities.data_classes import (
    event_source,
    APIGatewayProxyEvent,
)
logger = Logger(service="api")


@event_source(data_class=APIGatewayProxyEvent)
@logger.inject_lambda_context(correlation_id_path=correlation_paths.API_GATEWAY_REST)
def lambda_handler(event: APIGatewayProxyEvent, context: LambdaContext) -> dict:
    logger.info(
        f"Received request using {event.request_context.request_id} as correlation id"
    )

Here you see that there are 2 things that you need to maintain the correlation_id_path=correlation_paths.API_GATEWAY_REST and the event.request_context.request_id both need to be updated if you want to change it.

Proposal

Adding Optional[str] typing or a logger.remove_correlation_id() method would solve the removal of the correlation id.

A logger.get_correlation_id() method would help but from the discussions on slack that might not be so straight forward. @heitorlessa could you add some context?

If this feature should be available in other runtimes (e.g. Java, Typescript), how would this look like to ensure consistency? The interface should be the same but tailored to the language standards.

User Experience

Get the current correlation id:

@event_source(data_class=APIGatewayProxyEvent)
@logger.inject_lambda_context(correlation_id_path=correlation_paths.API_GATEWAY_REST)
def lambda_handler(event: APIGatewayProxyEvent, _: LambdaContext) -> dict:
    logger.info(
        f"Received request using {logger.get_correlation_id()} as correlation id"
    )

Remove the correlation id:

logger.set_correlation_id(None)
# or
logger.remove_correlation_id()

Drawbacks

Not that we know of until now

Rationale and alternatives

  • What other designs have been considered? Why not them? TBD
  • What is the impact of not doing this? It breaks the typing interface, python will not break on it but it's not nice

Unresolved questions

Optional, stash area for topics that need further development e.g. TBD

@heitorlessa
Copy link
Contributor

heitorlessa commented Jul 18, 2021

Thanks a lot for creating the RFC @Nr18 -- I'll re-read it and review the convo on Slack more carefully this week.

I'm happy with the getter but I'm on the fence on the remove method. If I'm reading this right at night, I think there are two things going on here:

1/ SQS correlation ID message attribute might not be exist, and it's correctly warning you it might not be what you want to set. If we had Optional[str], this could lead some customers to think Logger is at fault.

2/ While setting None will effectively remove from the logger it might not map correctly in other runtimes -- .remove_keys(["correlation_id"]) does what's expected here

set_correlation_id is largely a convenient method for append_keys(correlation_id=value), hence a getter makes total sense... perhaps even a generic one so you could retrieve any key.

As always, I truly appreciate the help and your support on fleshing this out.

@heitorlessa
Copy link
Contributor

Update July 19th: I've read the thread on Slack, here's my answer as well as both original samples revisited with comments.

I'm happy with the getter and there is no need for a specific method to remove correlation_id key, since we already have remove_keys["correlation_id"]. I'm also happy with changing the type to Optional[str], this would trigger Logger to remove correlation_id key if its value is None without any extra code.

SQS example revisited

from aws_lambda_powertools import Logger
from aws_lambda_powertools.utilities.typing import LambdaContext
from aws_lambda_powertools.utilities.batch import sqs_batch_processor
from aws_lambda_powertools.utilities.data_classes.sqs_event import SQSRecord


logger = Logger(service="callback")


def record_handler(record: dict) -> None:
    record = SQSRecord(record)
    # Get the correlation id from the message attributes
    correlation_id: Optional[str] = record.message_attributes["correlation_id"].string_value
    # NOTE:
    # If a correlation ID exists it'll overwrite, otherwise it'll set a new one with ^
    # Else, any key with a `None` value will be removed in the next log entry generation
    logger.set_correlation_id(correlation_id)
    logger.info(f"Processing message with {get_correlation_id} as correlation_id")


@sqs_batch_processor(record_handler=record_handler)
def lambda_handler(event: dict, context: LambdaContext) -> None:
    # logger.set_correlation_id(None)  # No need to do this here
    logger.info(f"Received a SQSEvent with {len(list(event.records))} records")

API Gateway example revisited

from aws_lambda_powertools import Logger
from aws_lambda_powertools.logging import correlation_paths
from aws_lambda_powertools.utilities.typing import LambdaContext
from aws_lambda_powertools.utilities.data_classes import (
    event_source,
    APIGatewayProxyEvent,
)
logger = Logger(service="api")


@event_source(data_class=APIGatewayProxyEvent)
@logger.inject_lambda_context(correlation_id_path=correlation_paths.API_GATEWAY_REST)
def lambda_handler(event: APIGatewayProxyEvent, context: LambdaContext) -> dict:
    logger.info(
        f"Received request using {logger.get_correlation_id()} as correlation id"
    )

@heitorlessa
Copy link
Contributor

Moving it to the official Roadmap repo now that's been approved, so we can keep track for other runtimes.

@heitorlessa heitorlessa transferred this issue from aws-powertools/powertools-lambda-python Jul 19, 2021
@heitorlessa heitorlessa added Logger Logger utility Proposed Community submited RFC labels Jul 19, 2021
@heitorlessa
Copy link
Contributor

Released in 1.18.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Logger Logger utility Proposed Community submited RFC
Projects
None yet
Development

No branches or pull requests

2 participants