-
Notifications
You must be signed in to change notification settings - Fork 45
Reduce allocations during event parsing. #463
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
Conversation
@@ -66,6 +66,8 @@ | |||
|
|||
telemetry_writer.enable() | |||
|
|||
is_lambda_context = os.environ.get(XrayDaemon.FUNCTION_NAME_HEADER_NAME) != "" |
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.
This logic used to be in a function. But since the function name header won't change between invocations, we can parse it just once at the global 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.
LGTM. Moreover, I actually overlooked allocations such as get("key", _some_default)
. We should probably summarize these things as a guide and add it to the PR template.
datadog_lambda/tracing.py
Outdated
dd_json_data = None | ||
dd_json_data_type = dd_payload.get("Type", dd_payload.get("dataType", "")) | ||
dd_json_data_type = dd_payload.get("Type") | ||
if dd_json_data_type is None: | ||
dd_json_data_type = dd_payload.get("dataType") | ||
if dd_json_data_type == "Binary": | ||
dd_json_data = dd_payload.get( | ||
"binaryValue", | ||
dd_payload.get("Value", r"{}"), | ||
) | ||
dd_json_data = base64.b64decode(dd_json_data) | ||
dd_json_data = dd_payload.get("binaryValue") | ||
if dd_json_data is None: | ||
dd_json_data = dd_payload.get("Value") | ||
if dd_json_data: | ||
dd_json_data = base64.b64decode(dd_json_data) | ||
elif dd_json_data_type == "String": | ||
dd_json_data = dd_payload.get( | ||
"stringValue", | ||
dd_payload.get("Value", r"{}"), | ||
) | ||
dd_json_data = dd_payload.get("stringValue") | ||
if dd_json_data is None: | ||
dd_json_data = dd_payload.get("Value") |
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 I like that we are reducing allocations, this is really hard to read 😢 maybe in the future when we split the files, this would make more sense
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.
Yes agreed. This is difficult to read. I'll take a stab at improving it a bit.
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.
Another PR is fine, I guess when this is separated, this can be much easier! Not a blocking comment
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.
@duncanista Okay, I made some updates. Let me know if this is any better for you.
28ad948
to
87242a8
Compare
What does this PR do?
Reduces allocations in all functions in the
datadog_lambda/tracing.py
file.Motivation
It's faster.
Testing Guidelines
Additional Notes
Before and after comparison. Note this only compares the parsing of lambda function urls. Therefore, other event types could see more/less performance improvements.
Types of Changes
Check all that apply