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

Collect trace_api.{requests,responses,errors} metrics in the background trace sender #2672

Merged
merged 14 commits into from
May 28, 2024

Conversation

iamluc
Copy link
Contributor

@iamluc iamluc commented May 23, 2024

Description

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@codecov-commenter
Copy link

codecov-commenter commented May 23, 2024

Codecov Report

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

Project coverage is 77.80%. Comparing base (f246102) to head (7bafb13).
Report is 3 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2672   +/-   ##
=========================================
  Coverage     77.79%   77.80%           
  Complexity     2223     2223           
=========================================
  Files           226      226           
  Lines         26276    26339   +63     
  Branches        988      988           
=========================================
+ Hits          20441    20492   +51     
- Misses         5309     5321   +12     
  Partials        526      526           
Flag Coverage Δ
appsec-extension 69.13% <ø> (ø)
tracer-extension 78.67% <81.95%> (+0.01%) ⬆️
tracer-php 80.33% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
ext/ddtrace.c 74.54% <100.00%> (ø)
ext/ddtrace.h 62.50% <ø> (ø)
ext/sidecar.c 88.18% <86.20%> (+4.85%) ⬆️
ext/telemetry.c 93.84% <81.57%> (-5.07%) ⬇️
ext/coms.c 78.80% <80.00%> (-0.82%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f246102...7bafb13. Read the comment docs.

@pr-commenter
Copy link

pr-commenter bot commented May 23, 2024

Benchmarks

Benchmark execution time: 2024-05-28 18:51:54

Comparing candidate commit 7bafb13 in PR branch luc/bgs-trace_api-metrics with baseline commit f246102 in branch master.

Found 8 performance improvements and 6 performance regressions! Performance is the same for 164 metrics, 0 unstable metrics.

scenario:ComposerTelemetryBench/benchTelemetryParsing-opcache

  • 🟥 execution_time [+2.076µs; +4.324µs] or [+3.472%; +7.231%]

scenario:EmptyFileBench/benchEmptyFileOverhead

  • 🟩 execution_time [-548.278µs; -392.942µs] or [-17.442%; -12.501%]

scenario:EmptyFileBench/benchEmptyFileOverhead-opcache

  • 🟩 execution_time [-580.249µs; -433.171µs] or [-17.783%; -13.276%]

scenario:HookBench/benchHookOverheadInstallHookOnMethod

  • 🟥 execution_time [+18.784µs; +52.465µs] or [+2.280%; +6.368%]

scenario:LaravelBench/benchLaravelOverhead

  • 🟩 execution_time [-469.967µs; -324.573µs] or [-14.469%; -9.993%]

scenario:LaravelBench/benchLaravelOverhead-opcache

  • 🟩 execution_time [-606.675µs; -444.065µs] or [-17.752%; -12.994%]

scenario:LogsInjectionBench/benchLogsInfoInjection-opcache

  • 🟥 mem_peak [+87.431KB; +238.810KB] or [+4.301%; +11.747%]

scenario:MessagePackSerializationBench/benchMessagePackSerialization-opcache

  • 🟩 execution_time [-6.397µs; -4.443µs] or [-4.106%; -2.852%]

scenario:PDOBench/benchPDOBaseline

  • 🟥 execution_time [+15.894µs; +16.773µs] or [+9.071%; +9.573%]

scenario:PDOBench/benchPDOOverhead-opcache

  • 🟥 execution_time [+13.009µs; +16.756µs] or [+4.658%; +5.999%]

scenario:PDOBench/benchPDOOverheadWithDBM

  • 🟩 execution_time [-17.562µs; -13.130µs] or [-5.616%; -4.199%]

scenario:PDOBench/benchPDOOverheadWithDBM-opcache

  • 🟥 execution_time [+14.123µs; +18.241µs] or [+4.582%; +5.918%]

scenario:SymfonyBench/benchSymfonyOverhead

  • 🟩 execution_time [-613.423µs; -574.957µs] or [-8.821%; -8.268%]

scenario:SymfonyBench/benchSymfonyOverhead-opcache

  • 🟩 execution_time [-584.712µs; -530.048µs] or [-8.271%; -7.497%]

@iamluc iamluc force-pushed the luc/bgs-trace_api-metrics branch 2 times, most recently from 41b71fc to 2e84fcd Compare May 27, 2024 12:57
Direct leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0xffff8dfed43b in malloc (/usr/lib/aarch64-linux-gnu/libasan.so.5+0xcf43b)
    #1 0xffff81eb8f4f in __cxa_thread_atexit_impl /home/circleci/datadog/tmp/build_extension/ext/ddtrace.c:560
    #2 0xffff82ed7c8b in std::sys::unix::thread_local_dtor::register_dtor::ha7abe21b2e8f0491 library/std/src/sys/unix/thread_local_dtor.rs:31
    #3 0xffff81ec332b in _dd_writer_loop /home/circleci/datadog/tmp/build_extension/ext/coms.c:1053
    #4 0xffff8db7a7e3 in start_thread /build/glibc-tVuo8E/glibc-2.28/nptl/pthread_create.c:486
    #5 0xffff8b16a70b  (/lib/aarch64-linux-gnu/libc.so.6+0xcf70b)

SUMMARY: AddressSanitizer: 24 byte(s) leaked in 1 allocation(s).
@iamluc iamluc mentioned this pull request May 27, 2024
2 tasks
@iamluc iamluc changed the base branch from luc/sidecar-thread-safety to master May 27, 2024 13:47
@iamluc iamluc force-pushed the luc/bgs-trace_api-metrics branch from 2e84fcd to 42d8f71 Compare May 27, 2024 13:54
@iamluc iamluc force-pushed the luc/bgs-trace_api-metrics branch from 42d8f71 to 369a6d3 Compare May 27, 2024 13:55
@iamluc iamluc marked this pull request as ready for review May 27, 2024 16:08
@iamluc iamluc requested review from a team as code owners May 27, 2024 16:08
ext/telemetry.c Outdated Show resolved Hide resolved
bwoebi and others added 2 commits May 27, 2024 22:42
Copy link
Collaborator

@pierotibou pierotibou left a comment

Choose a reason for hiding this comment

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

LGTM funtionnaly speaking, just a comment about retries

break;
}

// Collect metrics for the last performed HTTP query
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we count the retries (in the for loop) as well ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it's the same logic that in the sidecar. See DataDog/libdatadog#418 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed, forgot about that, thanks for raising. Though, I'm not sure it's the right move to make... Indeed, if we want to be able to compare, we need to make sure this is how things have been implemented in other languages. The thing is, maybe all languages have implemented it differently, so maybe having a simple solution works and we just need to know the differences. Anyway, let's discuss that with everyone.

@iamluc iamluc force-pushed the luc/bgs-trace_api-metrics branch from 5a0ba6a to dd04d06 Compare May 28, 2024 09:14
@iamluc iamluc force-pushed the luc/bgs-trace_api-metrics branch from dd04d06 to f43712a Compare May 28, 2024 10:16
@bwoebi bwoebi force-pushed the luc/bgs-trace_api-metrics branch from c85afcc to a93e2a5 Compare May 28, 2024 12:23
@bwoebi bwoebi force-pushed the luc/bgs-trace_api-metrics branch from 739fb45 to 93fe542 Compare May 28, 2024 13:03
@bwoebi bwoebi force-pushed the luc/bgs-trace_api-metrics branch from 172bb19 to df4f69c Compare May 28, 2024 13:43
Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
@bwoebi bwoebi force-pushed the luc/bgs-trace_api-metrics branch from df4f69c to 57bff33 Compare May 28, 2024 15:25
Copy link
Collaborator

@bwoebi bwoebi left a comment

Choose a reason for hiding this comment

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

Things seem green now :-)

@bwoebi bwoebi force-pushed the luc/bgs-trace_api-metrics branch from 0420b4d to eebd844 Compare May 28, 2024 16:50
Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
@bwoebi bwoebi force-pushed the luc/bgs-trace_api-metrics branch from eebd844 to 7bafb13 Compare May 28, 2024 18:24
@DataDog DataDog deleted a comment from datadog-datadog-prod-us1 bot May 28, 2024
@bwoebi bwoebi merged commit 6307408 into master May 28, 2024
603 of 606 checks passed
@bwoebi bwoebi deleted the luc/bgs-trace_api-metrics branch May 28, 2024 20:07
@github-actions github-actions bot added this to the 1.1.0 milestone May 28, 2024
@bwoebi bwoebi modified the milestones: 1.1.0, 1.0.0 May 29, 2024
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.

4 participants