-
Notifications
You must be signed in to change notification settings - Fork 45
feat: add peer.service
#337
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: add peer.service
#337
Conversation
What about adding peer.service on the function execution span? |
This would have to be handled by |
Interesting, so you're saying in the tracer we'd add peer.service on the actual function execution span? Doing this in a different pr is fine with me but what you said doesn't make sense to me since the function execution span is created here so why would we want to add the peer.service tag in the tracer instead of here in dd lambda py? Unless it's because any downstream spans in relation to the function execution span are created in the tracer thus I'd have the appropriate value of what the peer.service tag should equal I.e. if we have a function that makes an aws sdk request to sqs then because that aws sdk request span is created in the tracer we would add another tag to the function execution span pointing to the sqs queue??? |
I wonder how we would edit the function execution span in the tracer. For some reason that isn't clicking, I assume we start and finish it in dd lambda py so adding that peer.service span in the tracer after its been finished seems like it may be difficult but I've never done that. Lmk |
datadog_lambda/tracing.py
Outdated
@@ -801,6 +808,7 @@ def create_inferred_span_from_api_gateway_websocket_event( | |||
"connection_id": request_context.get("connectionId"), | |||
"event_type": request_context.get("eventType"), | |||
"message_direction": request_context.get("messageDirection"), | |||
"peer.service": service_env_var, |
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 a general comment and it doesn't have to be done in this pr: we should create a general_tags()
method and save some repetition.
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.
I agree, we should refactor this, but it's a really long file. We should follow a similar approach in Node.
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
this should be handled in a profiling PR
@@ -315,6 +317,9 @@ def _after(self, event, context): | |||
if status_code: | |||
self.inferred_span.set_tag("http.status_code", status_code) | |||
|
|||
if self.service: | |||
self.inferred_span.set_tag("peer.service", self.service) |
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.
nice
What does this PR do?
Adds the field peer.service to inferred spans.
The field is set to the current process environment variable DD_SERVICE.
Motivation
Add another dimension to services in APM.
Testing Guidelines
Additional Notes
Refactoring in
tracing.py
is needed.Types of Changes
Check all that apply