datadog-exporter: don't set tags which are None #272

alertedsnake · 2020-12-22T22:58:49Z

Description

This is often None, but tags are always strings, and so things get broken when spans get passed along to other client calls.

Fixes #262

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

This passes existing tests, and I've added one to verify when the field is set to None, it's skipped.

Does This PR Require a Core Repo Change?

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

alertedsnake · 2021-01-14T21:02:37Z

One of the tests failed with a "performance alert" when I merged master in, not sure what that's about?

srikanthccv · 2021-01-29T15:28:42Z

exporter/opentelemetry-exporter-datadog/src/opentelemetry/exporter/datadog/propagator.py

@@ -97,7 +97,7 @@ def inject(
            self.SAMPLING_PRIORITY_KEY,
            str(constants.AUTO_KEEP if sampled else constants.AUTO_REJECT),
        )
-        if constants.DD_ORIGIN in span.context.trace_state:
+        if span.context.trace_state.get(constants.DD_ORIGIN) is not None:


I believe this will not happen from the latest version. Earlier TraceState was simple dict and there were no validation upon add/update key-value pair. We have fixed that in latest release and key-value pair much adhere to W3C tracestate format rules otherwise they will be discarded.

Ah very nice. I'll have to see if I can recreate the scenario I described in #262 with the current versions. Though, I am a little concerned that by disallowing that to be None we'll have moved the error from this code I patched, to whatever code was actually causing it in the first place.

Okay, rebased on the current main branch, and after some more testing, this PR still fixes the issue described in #262, so whatever change you're describing did not fix things.

That's strange that you are still running into that error. From here every value in the tracestate must be a str type.

Yeah, and I still haven't figured out where it's coming from.

Did you update the opentelemetry-api and propagator versions you are using? Can you provide a reproducible example?

Yep, this is with code from the main branch of both main and contrib repositories.

I'll see if I can strip down something - so far I've only seen when making gRPC client calls to a gRPC service running the botocore instrumentation and datadog exporter, so it's a little complex to reproduce, but I might be able to simplify more. Give me a day or two.

alertedsnake · 2021-02-14T22:42:54Z

Aha, so I've managed to nearly re-recreate the scenario in a simplified way, and I've seen this message in the log:

WARNING:opentelemetry.trace.span:Invalid key/value pair (dd_origin, ) found.

I assume this is the thing you were talking about @lonewolf3739 - it does make my patch not necessary. So it's quite possible this is fixed, let me recreate all of my virtualenvs and pull in the latest, I must have missed something.

edit: actually what I should be doing is trying to find the source of that log message ^ - clearly that's the actual bug.

lzchen · 2021-02-24T01:56:58Z

@alertedsnake
Any updates on this?

alertedsnake · 2021-02-24T02:01:10Z

@lzchen yeah, there's some more detail in #262 - at this point I have found where the problem happens, but I don't really know the intent of the code that's setting this field to None, was asking if you guys knew more about what it's supposed to be doing.

This comment specifically: #262 (comment)

alertedsnake · 2021-03-02T14:59:00Z

This fix no longer seems necessary as there isn't an exception thrown anymore, but the underlying bug does still seem present, see further discussion in #262 .

alertedsnake · 2021-04-07T03:26:28Z

Hi, so I'm going to re-open this, since it does actually fix an issue I'm finding with None making it into HTTP headers being sent via botocore requests;

TypeError: expected string or bytes-like object

The header that's incorrect looks like this:

'x-datadog-origin': None,

The code in this PR does indeed fix the issue. But again, this is only fixing an incorrect issue from propagating, it's not fixing the issue from happening - see all of the previous discussion about that.

srikanthccv · 2021-04-07T13:06:39Z

Can you share some minimal reproducible example for this problem? I don't see the possibility of tracestate value to be set to None.

alertedsnake · 2021-04-07T13:22:41Z

As I mentioned, there's a lot of previous discussion wherein I went through this in detail, both here and in the linked issue. In this case, I've got a gRPC service that makes a query to DynamoDB, and it's when that Dynamo call gets made (with the botocore instrumentation) that one of the HTTP headers on the request is 'x-datadog-origin': None.

In this message I mention the code that causes the problem and asked for some insight from others who have worked on this - getting an answer here would help quite a bit:
#262 (comment)

There isn't really a minimal case, I could probably make one up if necessary, but I think it's pretty clear where the problem occurs from that question.

srikanthccv · 2021-04-07T15:03:11Z

I haven't followed that discussion closely. If this indeed fixes the problem then there is a possibility that same problem exists in other propagators as well. If you could provide some reproducible example then we could find the root of problem and fix that instead of adding checks to prevent None getting propagated.

I don't think this fix changes anything by looking at theclass TraceState(typing.Mapping[str, str]): implementation unless I am missing something.

alertedsnake · 2021-04-07T17:55:04Z

Here is my test case: https://github.com/alertedsnake/otel-test with some instructions on how to run it and how to discover the problem. Let me know if this helps.

srikanthccv · 2021-04-08T15:17:07Z

Here is my test case: https://github.com/alertedsnake/otel-test with some instructions on how to run it and how to discover the problem. Let me know if this helps.

Thanks, I will check it.

srikanthccv · 2021-04-10T17:39:59Z

Here is my test case: https://github.com/alertedsnake/otel-test with some instructions on how to run it and how to discover the problem. Let me know if this helps.

Looks like I need api keys/creds of various service providers otherwise I won't be able to run this example. I am fine with this getting in although I think this doesn't really fix the underlying problem. @alertedsnake Did you try pip editable install or something similar to see if this really fixed your issue?

alertedsnake · 2021-04-10T18:03:25Z

Yeah, you'll need data dog keys, or to use one of the agent mocking Docker images, I can try to get that working.

As for the fix in this PR, yes, it does prevent the exception. But yes, it doesn't fix the actual problem - I was still hoping to get some insight from the original author or someone who knows the suspect code well.

alertedsnake · 2021-04-12T14:30:38Z

@lonewolf3739 Ok I updated the test case to not require Datadog credentials, and added a line to the readme about the AWS credentails - apparently you don't need either to see the error happen.

srikanthccv · 2021-04-14T17:02:28Z

@alertedsnake Thanks for sharing the updated example. I have done the troubleshooting and found the underlying problem. This line if constants.DD_ORIGIN in span.context.trace_state: evaluates to true even when there is no dd_origin key. The TraceState doesn't implement the __contains__ so the fallback as described in python docs returns true.

For user-defined classes which do not define contains() but do define iter(), x in y is True if some value z, for which the expression x is z or x == z is true, is produced while iterating over y. If an exception is raised during the iteration, it is as if in raised that exception.

Lastly, the old-style iteration protocol is tried: if a class defines getitem(), x in y is True if and only if there is a non-negative integer index i such that x is y[i] or x == y[i], and no lower integer index raises the IndexError exception. (If any other exception is raised, it is as if in raised that exception).

And this line span.context.trace_state[constants.DD_ORIGIN], returns None which is the reason for this problem.

alertedsnake · 2021-04-14T17:14:06Z

This line if constants.DD_ORIGIN in span.context.trace_state: evaluates to true even when there is no dd_origin key. The TraceState doesn't implement the __contains__ so the fallback as described in python docs returns true.

This is the kind of thing I was hoping someone with more knowledge of OTEL could figure out, nice work! I guess this means my fix in this PR is the actual fix, I thought I was just fixing the symptom.

This is often `None`, but tags are always strings, and so things get broken when spans get passed along to other client calls. Fixes open-telemetry#262

srikanthccv · 2021-04-14T18:36:14Z

No this is not the actual fix. We need to change tracestate to behave correctly with membership. Once we do that this is no longer needed.

alertedsnake · 2021-04-14T18:40:54Z

Okay, that sounds even better. I'm happy to close this in favor of a ticket in the main repo.

alertedsnake · 2021-04-14T19:00:56Z

Okay, that sounds even better. I'm happy to close this in favor of a ticket in the main repo.

In fact, I've got a fix, will make a PR shortly.

alertedsnake · 2021-04-14T19:19:01Z

open-telemetry/opentelemetry-python#1773 - I don't think it needs anything more than this, but let me know if so.

Happy to open an issue in that repo if we think it needs one.

lzchen · 2021-04-16T15:39:45Z

@lonewolf3739
Is this PR still relevant now that this is merged?

srikanthccv · 2021-04-16T16:11:58Z

This can be closed.

alertedsnake force-pushed the datadog-propagator-bugfix branch from 2a58e32 to 1cbc87d Compare December 23, 2020 02:07

alertedsnake marked this pull request as ready for review December 23, 2020 03:31

alertedsnake requested review from a team, codeboten and lzchen and removed request for a team December 23, 2020 03:31

srikanthccv reviewed Jan 29, 2021

View reviewed changes

Base automatically changed from master to main January 29, 2021 19:28

alertedsnake force-pushed the datadog-propagator-bugfix branch from 5e4b639 to 206eafa Compare February 4, 2021 18:06

alertedsnake force-pushed the datadog-propagator-bugfix branch 2 times, most recently from 926fcba to da614b6 Compare February 14, 2021 19:18

alertedsnake force-pushed the datadog-propagator-bugfix branch from da614b6 to 98a3a65 Compare March 2, 2021 14:54

alertedsnake closed this Mar 2, 2021

This was referenced Mar 10, 2021

gRPC client + botocore + datadog exporter instrumentation error #262

Closed

datadog: do not add None tracestate value. #368

Merged

alertedsnake reopened this Apr 7, 2021

alertedsnake force-pushed the datadog-propagator-bugfix branch from 98a3a65 to d4095d0 Compare April 7, 2021 03:29

codeboten requested a review from srikanthccv April 14, 2021 15:48

Michael Stella added 4 commits April 14, 2021 14:07

Don't set tags which are None

b31a71e

This is often `None`, but tags are always strings, and so things get broken when spans get passed along to other client calls. Fixes open-telemetry#262

changelog

60ac198

add extra test

e5d958c

lint

1a50126

alertedsnake force-pushed the datadog-propagator-bugfix branch from d4095d0 to 1a50126 Compare April 14, 2021 18:16

lzchen closed this Apr 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

datadog-exporter: don't set tags which are None #272

datadog-exporter: don't set tags which are None #272

alertedsnake commented Dec 22, 2020 •

edited

Loading

alertedsnake commented Jan 14, 2021

srikanthccv Jan 29, 2021

alertedsnake Jan 29, 2021

alertedsnake Feb 12, 2021

lzchen Feb 12, 2021

alertedsnake Feb 12, 2021

srikanthccv Feb 13, 2021

alertedsnake Feb 13, 2021

alertedsnake commented Feb 14, 2021 •

edited

Loading

lzchen commented Feb 24, 2021

alertedsnake commented Feb 24, 2021

alertedsnake commented Mar 2, 2021

alertedsnake commented Apr 7, 2021

srikanthccv commented Apr 7, 2021

alertedsnake commented Apr 7, 2021

srikanthccv commented Apr 7, 2021

alertedsnake commented Apr 7, 2021

srikanthccv commented Apr 8, 2021

srikanthccv commented Apr 10, 2021

alertedsnake commented Apr 10, 2021

alertedsnake commented Apr 12, 2021

srikanthccv commented Apr 14, 2021

alertedsnake commented Apr 14, 2021 •

edited

Loading

srikanthccv commented Apr 14, 2021

alertedsnake commented Apr 14, 2021

alertedsnake commented Apr 14, 2021

alertedsnake commented Apr 14, 2021

lzchen commented Apr 16, 2021

srikanthccv commented Apr 16, 2021

datadog-exporter: don't set tags which are None #272

datadog-exporter: don't set tags which are None #272

Conversation

alertedsnake commented Dec 22, 2020 • edited Loading

Description

Type of change

How Has This Been Tested?

Does This PR Require a Core Repo Change?

Checklist:

alertedsnake commented Jan 14, 2021

srikanthccv Jan 29, 2021

Choose a reason for hiding this comment

alertedsnake Jan 29, 2021

Choose a reason for hiding this comment

alertedsnake Feb 12, 2021

Choose a reason for hiding this comment

lzchen Feb 12, 2021

Choose a reason for hiding this comment

alertedsnake Feb 12, 2021

Choose a reason for hiding this comment

srikanthccv Feb 13, 2021

Choose a reason for hiding this comment

alertedsnake Feb 13, 2021

Choose a reason for hiding this comment

alertedsnake commented Feb 14, 2021 • edited Loading

lzchen commented Feb 24, 2021

alertedsnake commented Feb 24, 2021

alertedsnake commented Mar 2, 2021

alertedsnake commented Apr 7, 2021

srikanthccv commented Apr 7, 2021

alertedsnake commented Apr 7, 2021

srikanthccv commented Apr 7, 2021

alertedsnake commented Apr 7, 2021

srikanthccv commented Apr 8, 2021

srikanthccv commented Apr 10, 2021

alertedsnake commented Apr 10, 2021

alertedsnake commented Apr 12, 2021

srikanthccv commented Apr 14, 2021

alertedsnake commented Apr 14, 2021 • edited Loading

srikanthccv commented Apr 14, 2021

alertedsnake commented Apr 14, 2021

alertedsnake commented Apr 14, 2021

alertedsnake commented Apr 14, 2021

lzchen commented Apr 16, 2021

srikanthccv commented Apr 16, 2021

alertedsnake commented Dec 22, 2020 •

edited

Loading

alertedsnake commented Feb 14, 2021 •

edited

Loading

alertedsnake commented Apr 14, 2021 •

edited

Loading