DataDog / nginx-datadog

Enhance NGINX Observability and Security with Datadog's Module
https://www.datadoghq.com
Apache License 2.0
25 stars 10 forks source link

feat: use hex representation for `$datadog_trace_id` and `$datadog_parent_id` variables #103

Closed dmehala closed 3 months ago

dmehala commented 3 months ago

Changes:

Resolves #102

dgoffredo commented 3 months ago

Flyby comment. Maybe I should disable notifications about these repositories. :)

This change could cause problems for users who depend on those values currently and then upgrade.

Consider adding new variables for the hex representation. You probably already thought of that.

Maximal solution:

  1. $datadog_trace_id_hex is the 128-bit (!) trace ID in hexadecimal.
  2. $datadog_span_id_hex is the 64-bit span ID in hexadecimal.
  3. $datadog_trace_id_dec is the 64-bit (!) trace ID in decimal.
  4. $datadog_span_id_dec is the 64-bit span ID in decimal.
  5. $datadog_trace_id is $datadog_trace_id_dec.
  6. $datadog_span_id is $datadog_span_id_dec.

The tradeoff is between "backward compatibility" and "OpenTelemetry is the way." If 128-bit hex IDs really are where Datadog is going, then consider the above but with (5) and (6) changed to prefer hex, so that then you can print a big warning on the next release: "Trace ID and span ID variables are now hexadecimal! Use $foo_dec for the old behavior!"

Bear in mind also that these variables are baked into the default log format, so changing the meaning of the variables will affect the access logs of users who never typed the variable name.

dmehala commented 3 months ago

Hey @dgoffredo ! Thanks for your input.

I did considered new variables, but the more I reflect on it, the less certain I am that we actually need them. My thinking is $datadog_X_id doesn't serve much purpose for customers beyond log correlation or logging - though I could be wrong.

In the past, when I made a breaking change without notifying our customers, we received some complaints. So, I can almost hear Caleb's voice saying something along the line "you should warn first any future breaking change". Since you're suggesting the same, it's probably wise to follow that approach.

Thanks again!

dmehala commented 3 months ago

Hm, I see your point.

I will define $datadog_x_hex and log a warning that the default behaviour of $datadog_x will change in the N+1 release. Thank you for your feedback @pablomartinezbernardo .

codecov-commenter commented 3 months ago

Codecov Report

Attention: Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 66.95%. Comparing base (64a3c93) to head (fe5d408). Report is 1 commits behind head on master.

Files Patch % Lines
src/tracing_library.cpp 20.00% 2 Missing and 2 partials :warning:
Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/DataDog/nginx-datadog/pull/103/graphs/tree.svg?width=650&height=150&src=pr&token=SZCZI1FAYU&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog)](https://app.codecov.io/gh/DataDog/nginx-datadog/pull/103?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog) ```diff @@ Coverage Diff @@ ## master #103 +/- ## ========================================== - Coverage 67.00% 66.95% -0.05% ========================================== Files 35 35 Lines 3458 3465 +7 Branches 604 606 +2 ========================================== + Hits 2317 2320 +3 - Misses 824 826 +2 - Partials 317 319 +2 ``` | [Files](https://app.codecov.io/gh/DataDog/nginx-datadog/pull/103?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog) | Coverage Δ | | |---|---|---| | [src/datadog\_variable.cpp](https://app.codecov.io/gh/DataDog/nginx-datadog/pull/103?src=pr&el=tree&filepath=src%2Fdatadog_variable.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog#diff-c3JjL2RhdGFkb2dfdmFyaWFibGUuY3Bw) | `75.35% <100.00%> (+0.53%)` | :arrow_up: | | [src/tracing\_library.cpp](https://app.codecov.io/gh/DataDog/nginx-datadog/pull/103?src=pr&el=tree&filepath=src%2Ftracing_library.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog#diff-c3JjL3RyYWNpbmdfbGlicmFyeS5jcHA=) | `85.54% <20.00%> (-4.34%)` | :arrow_down: |