-
Notifications
You must be signed in to change notification settings - Fork 45
re 8000 factor: remove tracer dependencies to support dsm sqs -> lambda #612
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?
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
) | ||
except Exception as e: | ||
logger.error(format_err_with_traceback(e)) | ||
arn = record.get("eventSourceARN", "") |
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.
we remove the try / except here. Is there a reason for that (maybe there is and I don't see it).
But we want to make sure our instrumentation never prevents the lambda from being executed, even if there is an issue with the instrumentation.
context_json = _get_dsm_context_from_lambda(record) | ||
if not context_json: | ||
logger.debug("DataStreams skipped lambda message: %r", record) | ||
return None |
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.
just return is enough
return None | ||
|
||
carrier_get = _create_carrier_get(context_json) | ||
set_consume_checkpoint("sqs", arn, carrier_get) |
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.
(Let's make sure we update the set manual_checkpoint=False here once the other PR is merged & the tracer released.
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.
Plan on keeping this PR in draft until tracer released, and will make sure to update
- message.messageAttributes._datadog.stringValue (SQS -> lambda) | ||
""" | ||
context_json = None | ||
message_attributes = message.get("messageAttributes") |
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.
is it possible for the message attributes to be in the message body (for the sns --> sqs case for example)? Is that maybe in a separate PR?
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 will be in separate PR. I am trying to keep this one solely based on fixing the sqs logic
What does this PR do?
Removes dependencies on internal tracer code. The get_dsm_context() is moved inside the lambda layer, and the public facing api for setting checkpoints is used instead of internal implementation code in the tracer.
Motivation
Helps decouple the lambda layer code from the tracer code, keeps with bests practice of not using internal implementation code.
Testing Guidelines
Function was properly unit tested
Additional Notes
Types of Changes
Check all that apply