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

Conversation

ericmustin
Copy link
Contributor

Description

This PR fixes the service_name resolution for the datadog exporter. The order of operations here is:

  1. Resource attribute service.name
  2. User supplied datadog service

However, in practice due to the work done in this PR: open-telemetry/opentelemetry-python#1480

The resource attribute resolution will provide a default resource service.name of "unknown_service" in the event no resource attribute has been set for service.name. This means that even when a user does supply a fallback datadog service it will never be discovered beacuse the default "unknown_service" will take precedence. This PR updates the datadog exporter to explicitly check if a default unknown_service is being used for resource attribute service.name and if it does, allows the datadog supplied service to override it.

Fixes # (issue)

Internall bug reports impacting end users

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test A

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@ericmustin ericmustin requested review from a team, owais and aabmass and removed request for a team July 8, 2021 19:22
Copy link
Contributor

@NathanielRN NathanielRN left a comment

Choose a reason for hiding this comment

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

Minor suggestions, otherwise looks good to me 😄

@@ -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!


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".

CHANGELOG.md Outdated
@@ -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

CHANGELOG.md Outdated Show resolved Hide resolved
@lzchen
Copy link
Contributor

lzchen commented Jul 9, 2021

Both jaeger and zipkin do not allow users to set the service_name through the constructor. Should we be consistent and recommend users to set the service_name through a Resource?

@ericmustin
Copy link
Contributor Author

ericmustin commented Jul 9, 2021

@lzchen yes generally speaking I think that's a good direction to take, as I'd like to nudge folks to use the opentelemetry semantic conventions when using opentelemetry libraries. If that means removing the service argument for this exporter that's fine with me.

My only blocker is that I would have to coordinate with an update to the docs/blog posts/etc on my employers doc website so that any onboarding users were using the correct code snippets. So, if possible, i would prefer if we could just ship this non-breaking change (all the feedback seems reasonable, will update), and then I can try to get everything in order to update the exporter to be in line with other non-otlp exporters. If you'd like to make an issue for that work and assign it to me so that we can make sure this is tracked/completed, that's good with me.

Is that approach ok for y'all?

@lzchen
Copy link
Contributor

lzchen commented Jul 9, 2021

@ericmustin
Sure that sounds good. Please add a TODO to that section to indicate that we plan on changing to using the Resource.
I've created an issue here. Please comment on it so I can assign you.

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

Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

Could you add a test for this?

@ericmustin
Copy link
Contributor Author

@lzchen sorry for delay was on pto, added a test lmk if anything else needed here

@lzchen lzchen merged commit f6ffa76 into open-telemetry:main Jul 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants