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

chore(distributed_tracing): ensure last datadog parent id tag is always set #9174

Merged
merged 4 commits into from
May 8, 2024

Conversation

mabdinur
Copy link
Contributor

@mabdinur mabdinur commented May 6, 2024

Implements W3C Phase 3. The last datadog parent id is always propagated in a distributed trace (if ddtrace propagation style includes tracecontext). This change will reduce instances where flamegraphs contain missing spans.

Note - This change does not require a release note. It is a follow up to a previous PR.

Follow up to: 90a3e3f#diff-18af2a7682cf3014ab4ae236fc54d2454bbf5293e81e098b1d22d39bdb61012d

Checklist

  • Change(s) are motivated and described in the PR description
  • Testing strategy is described if automated tests are not included in the PR
  • Risks are described (performance impact, potential for breakage, maintainability)
  • Change is maintainable (easy to change, telemetry, documentation)
  • Library release note guidelines are followed or label changelog/no-changelog is set
  • Documentation is included (in-code, generated user docs, public corp docs)
  • Backport labels are set (if applicable)
  • If this PR changes the public interface, I've notified @DataDog/apm-tees.

Reviewer Checklist

  • Title is accurate
  • All changes are related to the pull request's stated goal
  • Description motivates each change
  • Avoids breaking API changes
  • Testing strategy adequately addresses listed risks
  • Change is maintainable (easy to change, telemetry, documentation)
  • Release note makes sense to a user of the library
  • Author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment
  • Backport labels are set in a manner that is consistent with the release branch maintenance policy

@mabdinur mabdinur requested a review from a team as a code owner May 6, 2024 21:13
@mabdinur mabdinur added the changelog/no-changelog A changelog entry is not required for this PR. label May 6, 2024
@datadog-dd-trace-py-rkomorn
Copy link

datadog-dd-trace-py-rkomorn bot commented May 6, 2024

Datadog Report

Branch report: munir/w3c-changes-2-3
Commit report: 4c903e6
Test service: dd-trace-py

✅ 0 Failed, 129678 Passed, 42213 Skipped, 3h 40m 38.59s Total duration (6h 34m 49.08s time saved)
❄️ 1 New Flaky

New Flaky Tests (1)

  • test_extract_traceparent[invalid_0_value_for_span_id] - test_propagation.py - Last Failure

    Expand for error
     'NoneType' object is not iterable
    

@brettlangdon
Copy link
Member

Note - This change does not require a release note. It is a follow up to a previous PR.

@mabdinur which PR?

@codecov-commenter
Copy link

Codecov Report

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

Project coverage is 6.74%. Comparing base (3ea8bd8) to head (db78bff).
Report is 5 commits behind head on main.

Files Patch % Lines
ddtrace/propagation/http.py 60.00% 2 Missing ⚠️
ddtrace/internal/constants.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #9174       +/-   ##
===========================================
- Coverage   78.55%    6.74%   -71.82%     
===========================================
  Files        1273     1244       -29     
  Lines      120300   118571     -1729     
===========================================
- Hits        94496     7992    -86504     
- Misses      25804   110579    +84775     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mabdinur
Copy link
Contributor Author

mabdinur commented May 6, 2024

Note - This change does not require a release note. It is a follow up to a previous PR.

@mabdinur which PR?

This change: 90a3e3f#diff-18af2a7682cf3014ab4ae236fc54d2454bbf5293e81e098b1d22d39bdb61012d.

@mabdinur mabdinur changed the title chore(w3c): ensure last datadog parent id is always set chore(distributed_tracing): ensure last datadog parent id tag is always set May 7, 2024
@mabdinur mabdinur enabled auto-merge (squash) May 7, 2024 15:25
Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

comment nit, otherwise lgtm

ddtrace/propagation/http.py Outdated Show resolved Hide resolved
ddtrace/propagation/http.py Outdated Show resolved Hide resolved
@pr-commenter
Copy link

pr-commenter bot commented May 7, 2024

Benchmarks

Benchmark execution time: 2024-05-07 20:51:00

Comparing candidate commit 4c903e6 in PR branch munir/w3c-changes-2-3 with baseline commit dac8aa2 in branch main.

Found 20 performance improvements and 27 performance regressions! Performance is the same for 162 metrics, 9 unstable metrics.

scenario:coreapiscenario-context_with_data_listeners

  • 🟥 max_rss_usage [+898.497KB; +966.821KB] or [+4.372%; +4.704%]

scenario:coreapiscenario-context_with_data_no_listeners

  • 🟩 max_rss_usage [-808.113KB; -599.272KB] or [-3.770%; -2.796%]

scenario:coreapiscenario-core_dispatch_listeners

  • 🟩 max_rss_usage [-802.151KB; -637.184KB] or [-3.736%; -2.968%]

scenario:coreapiscenario-core_dispatch_listeners_and_all_listeners

  • 🟥 max_rss_usage [+494.013KB; +709.802KB] or [+2.368%; +3.403%]

scenario:coreapiscenario-core_dispatch_no_listeners

  • 🟥 max_rss_usage [+660.192KB; +714.425KB] or [+3.269%; +3.538%]

scenario:coreapiscenario-core_dispatch_only_all_listeners

  • 🟩 max_rss_usage [-807.441KB; -647.867KB] or [-3.762%; -3.018%]

scenario:coreapiscenario-get_item_exists

  • 🟩 max_rss_usage [-586.797KB; -434.336KB] or [-2.741%; -2.029%]

scenario:coreapiscenario-get_item_missing

  • 🟩 max_rss_usage [-570.038KB; -517.040KB] or [-2.658%; -2.411%]

scenario:flasksimple-appsec-get

  • 🟥 max_rss_usage [+927.182KB; +1029.067KB] or [+2.722%; +3.021%]

scenario:flasksimple-appsec-post

  • 🟥 max_rss_usage [+867.455KB; +988.033KB] or [+2.547%; +2.901%]

scenario:httppropagationextract-b3_headers

  • 🟥 max_rss_usage [+536.343KB; +758.813KB] or [+2.586%; +3.659%]

scenario:httppropagationextract-datadog_tracecontext_tracestate_propagated_on_trace_id_match

  • 🟥 max_rss_usage [+665.699KB; +1179.139KB] or [+3.249%; +5.755%]

scenario:httppropagationextract-full_t_id_datadog_headers

  • 🟥 max_rss_usage [+451.752KB; +712.332KB] or [+2.166%; +3.415%]

scenario:httppropagationextract-invalid_priority_header

  • 🟥 max_rss_usage [+853.188KB; +1043.670KB] or [+4.163%; +5.092%]

scenario:httppropagationextract-invalid_span_id_header

  • 🟩 max_rss_usage [-1056.949KB; -648.216KB] or [-4.960%; -3.042%]

scenario:httppropagationextract-large_header_no_matches

  • 🟥 max_rss_usage [+0.891MB; +1.298MB] or [+4.393%; +6.398%]

scenario:httppropagationextract-large_valid_headers_all

  • 🟩 max_rss_usage [-948.203KB; -436.654KB] or [-4.489%; -2.067%]

scenario:httppropagationextract-medium_header_no_matches

  • 🟥 max_rss_usage [+1.196MB; +1.354MB] or [+5.937%; +6.723%]

scenario:httppropagationextract-medium_valid_headers_all

  • 🟩 max_rss_usage [-1070.577KB; -640.322KB] or [-5.021%; -3.003%]

scenario:httppropagationextract-none_propagation_style

  • 🟩 max_rss_usage [-702.260KB; -510.156KB] or [-3.281%; -2.384%]

scenario:httppropagationextract-valid_headers_basic

  • 🟥 max_rss_usage [+488.181KB; +727.921KB] or [+2.346%; +3.499%]

scenario:httppropagationextract-wsgi_invalid_priority_header

  • 🟥 max_rss_usage [+714.160KB; +763.268KB] or [+3.446%; +3.683%]

scenario:httppropagationextract-wsgi_invalid_span_id_header

  • 🟥 max_rss_usage [+0.862MB; +1.202MB] or [+4.252%; +5.928%]

scenario:httppropagationextract-wsgi_large_header_no_matches

  • 🟥 max_rss_usage [+1.024MB; +1.130MB] or [+5.028%; +5.548%]

scenario:httppropagationextract-wsgi_large_valid_headers_all

  • 🟩 max_rss_usage [-1063.778KB; -878.545KB] or [-4.980%; -4.113%]

scenario:httppropagationextract-wsgi_medium_header_no_matches

  • 🟥 max_rss_usage [+782.482KB; +1066.862KB] or [+3.830%; +5.222%]

scenario:httppropagationextract-wsgi_medium_valid_headers_all

  • 🟩 max_rss_usage [-1047.167KB; -821.838KB] or [-4.917%; -3.859%]

scenario:httppropagationinject-with_all

  • 🟩 max_rss_usage [-710.549KB; -673.080KB] or [-3.320%; -3.145%]

scenario:httppropagationinject-with_tags

  • 🟩 max_rss_usage [-625.448KB; -484.977KB] or [-2.928%; -2.270%]

scenario:httppropagationinject-with_tags_invalid

  • 🟩 max_rss_usage [-696.000KB; -660.186KB] or [-3.256%; -3.088%]

scenario:otelspan-start-finish

  • 🟥 max_rss_usage [+1.114MB; +1.170MB] or [+5.175%; +5.434%]

scenario:otelspan-start-finish-telemetry

  • 🟩 max_rss_usage [-787.252KB; -705.330KB] or [-3.453%; -3.093%]

scenario:sethttpmeta-all-disabled

  • 🟥 max_rss_usage [+609.337KB; +772.654KB] or [+2.894%; +3.669%]

scenario:sethttpmeta-all-enabled

  • 🟥 max_rss_usage [+428.191KB; +699.438KB] or [+2.031%; +3.318%]

scenario:sethttpmeta-collectipvariant_exists

  • 🟥 max_rss_usage [+689.833KB; +739.262KB] or [+3.279%; +3.514%]

scenario:sethttpmeta-obfuscation-send-querystring-disabled

  • 🟥 max_rss_usage [+453.530KB; +723.251KB] or [+2.132%; +3.399%]

scenario:sethttpmeta-obfuscation-worst-case-explicit-query

  • 🟥 max_rss_usage [+769.616KB; +992.893KB] or [+3.663%; +4.726%]

scenario:sethttpmeta-obfuscation-worst-case-implicit-query

  • 🟥 max_rss_usage [+603.357KB; +756.924KB] or [+2.835%; +3.557%]

scenario:sethttpmeta-useragentvariant_exists_1

  • 🟥 max_rss_usage [+667.176KB; +732.018KB] or [+3.168%; +3.476%]

scenario:sethttpmeta-useragentvariant_exists_2

  • 🟩 max_rss_usage [-733.997KB; -684.038KB] or [-3.371%; -3.142%]

scenario:sethttpmeta-useragentvariant_exists_3

  • 🟩 max_rss_usage [-717.730KB; -671.633KB] or [-3.299%; -3.087%]

scenario:sethttpmeta-useragentvariant_not_exists_1

  • 🟥 max_rss_usage [+437.083KB; +694.233KB] or [+2.075%; +3.296%]

scenario:span-start-finish

  • 🟩 max_rss_usage [-723.281KB; -670.588KB] or [-3.365%; -3.119%]

scenario:span-start-finish-telemetry

  • 🟥 max_rss_usage [+1.007MB; +1.176MB] or [+4.919%; +5.743%]

scenario:span-start-finish-traceid128

  • 🟥 max_rss_usage [+1.102MB; +1.154MB] or [+5.413%; +5.668%]

scenario:tracer-large

  • 🟩 max_rss_usage [-1.098MB; -1.017MB] or [-4.852%; -4.491%]

scenario:tracer-small

  • 🟩 max_rss_usage [-1.166MB; -1.084MB] or [-5.400%; -5.020%]

@mabdinur mabdinur merged commit 2230214 into main May 8, 2024
163 of 171 checks passed
@mabdinur mabdinur deleted the munir/w3c-changes-2-3 branch May 8, 2024 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog A changelog entry is not required for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants