-
Notifications
You must be signed in to change notification settings - Fork 49
feat: emit durable execution log to stdout at invocation start [SVLS-8582] #743
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,11 +2,17 @@ | |
| # under the Apache License Version 2.0. | ||
| # This product includes software developed at Datadog (https://www.datadoghq.com/). | ||
| # Copyright 2019 Datadog, Inc. | ||
| import json | ||
| import logging | ||
| import re | ||
| import sys | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| # When changing the schema of the durable invocation log, bump this version so | ||
| # the extension can handle compatibility between old and new schemas. | ||
| DURABLE_INVOCATION_LOG_SCHEMA_VERSION = "1.0.0" | ||
|
|
||
|
|
||
| def _parse_durable_execution_arn(arn): | ||
| """ | ||
|
|
@@ -47,3 +53,18 @@ def extract_durable_function_tags(event): | |
| "durable_function_execution_name": execution_name, | ||
| "durable_function_execution_id": execution_id, | ||
| } | ||
|
|
||
|
|
||
| def emit_durable_execution_log(request_id, execution_name, execution_id): | ||
| """ | ||
| Emits a structured JSON log to stdout mapping the Lambda request_id to the | ||
| durable execution name and ID. This is consumed by the Lambda extension layer | ||
| to correlate request IDs with durable executions. | ||
| """ | ||
| log = { | ||
| "request_id": request_id, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought extension side has the information of the request_id
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Race conditions often happen (e.g. logs can be delayed), and given a log message, it's hard for the extension to know which request_id it was for, so I think it's safer and easier to add request_id to the log message.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i see. that makes sense. |
||
| "durable_execution_name": execution_name, | ||
| "durable_execution_id": execution_id, | ||
| "schema_version": DURABLE_INVOCATION_LOG_SCHEMA_VERSION, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why we need a schema version here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm worried that we may need to add more fields in the future, though I'm not sure how yet. Adding schema version will make it easier for extension to identify incompatibility (e.g. missing field) and print an error to ask the user to update tracer.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. adding more fields shouldn't require a schema. i'm not very convinced that we need it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would agree that we don't need schema version. If we need to change this in the future, which I think is very unlikely, then we can add schema version at that point. |
||
| } | ||
| print(json.dumps(log), file=sys.stdout, flush=True) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not using the logger?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @joeyzhao2018 probably logger injects other things which we don't want
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this message should always be logged, and using logger introduces a risk that this message is dropped due to log level. Do you think it's safe to use
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, you mean the "parsing" part would be a big unpredictable? can we use regex to solve that part?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, i see, you don't want this to be controlled by log level. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although your change says "to use logs instead of traces to enrich logs" still creates a dependency where you need a tracing layer to have complete observability in logs – does that even make sense from a dependency standpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dependency refers to the one between trace agent and log agent in lambda extension. Using traces adds coupling between them, making extension code harder to maintain. See discussion here DataDog/datadog-lambda-extension#1053 (comment)