Skip to content
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

[exporter/datadog]: fix service name resolution #570

Merged
merged 15 commits into from
Jul 20, 2021
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([#560](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/560))
- `opentelemetry-instrumentation-django` Migrated Django middleware to new-style.
([#533](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/533))
- `opentelemetry-exporter-datadog` Fix service name resolution.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this describes the change more explicitly?

Suggested change
- `opentelemetry-exporter-datadog` Fix service name resolution.
- `opentelemetry-exporter-datadog` Datadog exporter should not use `unknown_service` as fallback resource service name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, updated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, updated

([#540](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/570))
ericmustin marked this conversation as resolved.
Show resolved Hide resolved

### Added
- `opentelemetry-instrumentation-httpx` Add `httpx` instrumentation
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,4 @@
DD_ERROR_TYPE_TAG_KEY = "error.type"
DD_ERROR_MSG_TAG_KEY = "error.msg"
DD_ERROR_STACK_TAG_KEY = "error.stack"
UNKNOWN_SERVICE_NAME = "unknown_service"
ericmustin marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
EXCEPTION_TYPE_ATTR_KEY,
SAMPLE_RATE_METRIC_KEY,
SERVICE_NAME_TAG,
UNKNOWN_SERVICE_NAME,
VERSION_KEY,
)
from opentelemetry.sdk.trace import sampling
Expand Down Expand Up @@ -135,12 +136,12 @@ def _translate_to_datadog(self, spans):
[
resource_tags,
resource_service_name,
] = _extract_tags_from_resource(span.resource)
] = _extract_tags_from_resource(span.resource, self.service)

datadog_span = DatadogSpan(
tracer,
_get_span_name(span),
service=resource_service_name or self.service,
service=resource_service_name,
resource=_get_resource(span),
span_type=_get_span_type(span),
trace_id=trace_id,
Expand Down Expand Up @@ -312,19 +313,23 @@ def _parse_tags_str(tags_str):
return parsed_tags


def _extract_tags_from_resource(resource):
def _extract_tags_from_resource(resource, fallback_service_name):
"""Parse tags from resource.attributes, except service.name which
has special significance within datadog"""
tags = {}
service_name = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's not used below on line 321 anymore, I think it would make sense to push the initialization of service_name = None lower to line 322 perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, updated!

if not (resource and getattr(resource, "attributes", None)):
return [tags, service_name]
return [tags, fallback_service_name]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically tags could just be {} here since it's not really use here at this point too! But that's not in scope for your PR I would say :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch. As you can tell my code is not particularly pythonic (i'm mostly ruby these days 😅 ). But there's a lot in this little exporter that could get cleaned up. I'll make a note to also try to give this code another pass for #574


for attribute_key, attribute_value in resource.attributes.items():
if attribute_key == SERVICE_NAME_TAG:
service_name = attribute_value
else:
tags[attribute_key] = attribute_value

if service_name is None or service_name == UNKNOWN_SERVICE_NAME:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It look like UNKNOWN_SERVICE_NAME is only used 1 time in this exporter? Because that's the case you could consider just doing "unknown_service" like we do in opentelemetry-sdk:

https://github.com/open-telemetry/opentelemetry-python/blob/f11ed2f3bacb11d53a7a2b4837cf6308fa34cc71/opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py#L172

But not a blocker!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either that or change both to use UNKNOWN_SERVICE_NAME

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, updated, i just removed the use of UNKNOWN_SERVICE_NAME entirely and went with "unknown_service".

service_name = fallback_service_name

return [tags, service_name]


Expand Down