`DD_TAGS` space separation

[dmehala]

Description

During my attempt to use nginx-datadog instead of the legacy nginx-opentracing module in system-tests. I found out dd-trace-cpp was crashing because of this odd DD_TAGS expression.

Let me save you a click (the link is here for context):

ENV DD_TAGS='key1:val1, key2 : val2 '

Investigation

I found this RFC from 2021 to support space separation in DD_TAGS.

Our implementation does not follow the recommend solution of this RFC and end up mixing spaces and commas. The result of our algorithm is:

While it should be as follow according to the RFC:

const std::vector<StringView> tags {
   {"key1:val1"},
   {"key2 : val2"}
};

[dgoffredo]

Yeah, I implemented the wrong algorithm.

Prior art in dd-opentracing-cpp:

• https://github.com/DataDog/dd-opentracing-cpp/blob/d07478e1bdecf3ea4f2d64c947e1aecad2c2cf40/doc/configuration.md?plain=1#L203
• https://github.com/DataDog/dd-opentracing-cpp/blob/d07478e1bdecf3ea4f2d64c947e1aecad2c2cf40/src/tracer_options.cpp#L215
• https://github.com/DataDog/dd-opentracing-cpp/blob/d07478e1bdecf3ea4f2d64c947e1aecad2c2cf40/src/tracer_options.cpp#L26
• https://github.com/DataDog/dd-opentracing-cpp/blob/d07478e1bdecf3ea4f2d64c947e1aecad2c2cf40/test/tracer_options_test.cpp#L196-L202

Not easy to see how it handles whitespace. I think it just considers it part of the tag.

I don't remember why I chose the unspecified "hybrid" approach.

Do you think we should follow the RFC? Perhaps it's best to do a survey of the other tracers. There's no relevant test in our internal parametric system tests.

[dmehala]

Yeah, I implemented the wrong algorithm.

You implemented based on what you had back then.

Do you think we should follow the RFC? Perhaps it's best to do a survey of the other tracers. There's no relevant test in our internal parametric system tests.

I did not find a system tests to ensure all tracers follow this RFC, but some of them are able to correctly interpret DD_TAGS='key1:val1, key2 : val2 ' because they pass this test

Upon investigation, at least 4 tracers (Python, Go, Ruby and Java) are following this RFC. I didn't find data for .NET, NodeJS and PHP.

IMO it's better to follow the RFC rather than crash the user application rejecting the configuration when spaces and commas are mixed.

EDIT: You are right, it's not a crash. My bad! 👀

[dgoffredo]

It's not a crash, it's a failure to start due to an invalid configuration:

2024/01/10 17:36:50 [notice] 38#38: signal process started
Sending the reload signal to nginx took 0.13857 seconds.
[6305.408870528, "service_log", {"service": "nginx", "payload": "2024/01/10 17:36:50 [error] 44#44: Failed to construct tracer: [error code 24] Unable to parse DD_TAGS environment variable: Unable to parse a key/value from the tag text \"key2\" because it does not contain the separator character \":\".  Error occurred in list of tags \"key1:val1, key2 : val2 \"."}]
Consuming docker compose top PIDs took 0.016283 seconds.
[6305.509158606, "service_log", {"service": "nginx", "payload": "2024/01/10 17:36:50 [alert] 1#1: worker process 44 exited with fatal code 2 and cannot be respawned"}]
Consuming docker compose top PIDs took 0.021696 seconds.

Pedantic, sure, but it makes me feel better.

Let's fix the implementation when we get the time.