exporter/datadog: add support for resource labels and service.name#1074
Conversation
exporter/opentelemetry-exporter-datadog/src/opentelemetry/exporter/datadog/exporter.py
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-datadog/src/opentelemetry/exporter/datadog/exporter.py
Outdated
Show resolved
Hide resolved
| if isinstance(attribute_value, (int, bool, float)): | ||
| value = str(attribute_value) | ||
| elif isinstance(attribute_value, str): | ||
| value = attribute_value | ||
| else: | ||
| logger.warning("Could not serialize tag %s", attribute_key) | ||
| continue | ||
|
|
There was a problem hiding this comment.
This could be more simply handled by attempting to cast to string inside a try/except block and log the exception if casting fails.
There was a problem hiding this comment.
makes sense, i had ripped it from the zipkin approach but agreed those improvements make sense to me
There was a problem hiding this comment.
turns out datadog's add span method handles this casting under the hood, so removing this section entirely for perf
|
just waiting on some internal review here, apols on the delay |
| has special significance within datadog""" | ||
| tags = {} | ||
| service_name = None | ||
| if not resource and resource.labels: |
There was a problem hiding this comment.
Did you mean if not (resource and resource.labels):
There was a problem hiding this comment.
👍 ah yup ty, updated
majorgreys
left a comment
majorgreys
left a comment
There was a problem hiding this comment.
This looks right to me. We can simplify the tag extraction and would benefit from having a few more tests around how resource and span attributes interact as well as how we had previously been configuring the exporter. Do we also want to explicitly deprecate passing a service name to the exporter and depend on the resource set in the tracer provider?
| return [tags, service_name] | ||
|
|
||
| for attribute_key, attribute_value in resource.labels.items(): | ||
| if isinstance(attribute_value, (int, bool, float)): |
There was a problem hiding this comment.
We can safely remove this logic since the exporter already makes use of ddtrace.span.Span.set_tag for handling span.attributes, which does the necessary casting based on the value or special conditions on keys:
https://github.com/DataDog/dd-trace-py/blob/master/ddtrace/span.py#L180-L259
There was a problem hiding this comment.
makes sense, updated
| resource = Resource( | ||
| labels={ | ||
| "key_resource": "some_resource", | ||
| "service.name": "resource_service_name", |
There was a problem hiding this comment.
Would be good to add a test for when span.attributes also has values for the same keys. Given the ordering above, span.attributes should override the resource labels.
There was a problem hiding this comment.
makes sense, added
| error=0, | ||
| service="test-service", | ||
| meta={"env": "test", "team": "testers", "version": "0.0.1"}, | ||
| service="resource_service_name", |
There was a problem hiding this comment.
Also want a test for when the span.resources does not include service.name and it's backed off to the existing configuration.
|
sorry for all the noise here, was having some linting issues |
…pen-telemetry#1074) * exporter/datadog: add support for resource labels and service.name
5 participants
Description
This PR adds support within the Datadog Exporter for capturing resource labels as datadog span tags as well as to set the service name using the more OpenTelemetry standard Resource
service.name. This aligns the exporter with the more broadly accepted OpenTelemetry standards and allows existing users of OpenTelemetry to leverage their existing patterns for service and span tagging if they choose to export to Datadog.cc @majorgreys for review and any comments here, if there's improvements or something I've missed please let me know.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
I've updated the Unit Tests as well as the Example
Checklist: