Provide a DogStatsD API by iamluc · Pull Request #2639 · DataDog/dd-trace-php

iamluc · 2024-04-23T08:44:35Z

Description

Provide a DogStatsD API.

Related libdatadog PR => DataDog/libdatadog#399

Reviewer checklist

Test coverage seems ok.
Appropriate labels assigned.

codecov-commenter · 2024-04-23T09:14:57Z

Codecov Report

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

Project coverage is 77.78%. Comparing base (1aaebd8) to head (51fbd86).
Report is 1 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2639      +/-   ##
============================================
+ Coverage     77.72%   77.78%   +0.06%     
  Complexity     2212     2212              
============================================
  Files           225      226       +1     
  Lines         26109    26250     +141     
  Branches        988      988              
============================================
+ Hits          20292    20418     +126     
- Misses         5291     5306      +15     
  Partials        526      526

Flag	Coverage Δ
appsec-extension	`69.13% <ø> (ø)`
tracer-extension	`78.67% <88.02%> (+0.11%)`	⬆️
tracer-php	`80.30% <ø> (ø)`

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

Files	Coverage Δ
ext/auto_flush.c	`56.17% <ø> (ø)`
ext/ddtrace.c	`74.54% <100.00%> (+0.88%)`	⬆️
ext/ddtrace.h	`62.50% <ø> (ø)`
ext/ddtrace_arginfo.h	`100.00% <ø> (ø)`
ext/sidecar.h	`53.84% <0.00%> (-9.80%)`	⬇️
ext/dogstatsd.c	`85.71% <85.71%> (ø)`
ext/sidecar.c	`83.33% <82.60%> (+0.57%)`	⬆️

... and 1 file with indirect coverage changes

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 1aaebd8...51fbd86. Read the comment docs.

pr-commenter · 2024-04-25T13:09:53Z

Benchmarks

Benchmark execution time: 2024-05-17 08:02:17

Comparing candidate commit 51fbd86 in PR branch lv/dogstatsd-api with baseline commit 1aaebd8 in branch master.

Found 14 performance improvements and 5 performance regressions! Performance is the same for 159 metrics, 0 unstable metrics.

scenario:EmptyFileBench/benchEmptyFileBaseline

🟩 execution_time [-455.369µs; -310.931µs] or [-16.175%; -11.045%]

scenario:EmptyFileBench/benchEmptyFileBaseline-opcache

🟩 execution_time [-451.791µs; -269.389µs] or [-15.526%; -9.258%]

scenario:EmptyFileBench/benchEmptyFileOverhead

🟩 execution_time [-498.069µs; -331.011µs] or [-13.965%; -9.281%]

scenario:EmptyFileBench/benchEmptyFileOverhead-opcache

🟩 execution_time [-555.972µs; -420.328µs] or [-14.965%; -11.314%]

scenario:LaravelBench/benchLaravelBaseline

🟩 execution_time [-421.136µs; -293.844µs] or [-14.220%; -9.922%]

scenario:LaravelBench/benchLaravelBaseline-opcache

🟩 execution_time [-452.796µs; -282.244µs] or [-14.736%; -9.185%]

scenario:LaravelBench/benchLaravelOverhead

🟩 execution_time [-559.835µs; -393.725µs] or [-14.806%; -10.413%]

scenario:LaravelBench/benchLaravelOverhead-opcache

🟩 execution_time [-548.354µs; -356.406µs] or [-14.184%; -9.219%]

scenario:MessagePackSerializationBench/benchMessagePackSerialization

🟥 execution_time [+4.904µs; +7.016µs] or [+3.326%; +4.758%]

scenario:MessagePackSerializationBench/benchMessagePackSerialization-opcache

🟥 execution_time [+4.430µs; +6.030µs] or [+2.960%; +4.028%]

scenario:PDOBench/benchPDOBaseline

🟥 execution_time [+16.761µs; +17.547µs] or [+9.593%; +10.043%]

scenario:PDOBench/benchPDOOverhead

🟥 execution_time [+15.542µs; +19.010µs] or [+5.696%; +6.966%]

scenario:PDOBench/benchPDOOverheadWithDBM

🟥 execution_time [+16.562µs; +18.151µs] or [+5.616%; +6.155%]

scenario:SymfonyBench/benchSymfonyBaseline

🟩 execution_time [-477.949µs; -430.871µs] or [-7.579%; -6.832%]

scenario:SymfonyBench/benchSymfonyBaseline-opcache

🟩 execution_time [-443.918µs; -372.242µs] or [-6.892%; -5.779%]

scenario:SymfonyBench/benchSymfonyOverhead

🟩 execution_time [-862.491µs; -786.849µs] or [-10.979%; -10.016%]

scenario:SymfonyBench/benchSymfonyOverhead-opcache

🟩 execution_time [-890.436µs; -838.324µs] or [-11.149%; -10.497%]

scenario:WordPressBench/benchWordPressOverhead

🟩 execution_time [-9.024ms; -7.319ms] or [-4.119%; -3.341%]

scenario:WordPressBench/benchWordPressOverhead-opcache

🟩 execution_time [-9.048ms; -7.530ms] or [-4.162%; -3.463%]

morrisonlevi · 2024-04-25T22:37:20Z

FYI there is a tiny dogstatsd client that should probably be cleaned up or deleted at src/dogstatsd.

….c is not compiled on windows

PROFeNoM · 2024-05-24T13:32:39Z

+    ddog_Endpoint *dogstatsd_endpoint;
    if (get_global_DD_TRACE_AGENTLESS() && ZSTR_LEN(get_global_DD_API_KEY())) {
        ddtrace_endpoint = ddog_endpoint_from_api_key(dd_zend_string_to_CharSlice(get_global_DD_API_KEY()));
+        dogstatsd_endpoint = ddog_endpoint_from_api_key(dd_zend_string_to_CharSlice(get_global_DD_API_KEY()));;
    } else {
        char *agent_url = ddtrace_agent_url();
        ddtrace_endpoint = ddog_endpoint_from_url((ddog_CharSlice) {.ptr = agent_url, .len = strlen(agent_url)});
        free(agent_url);
+
+        char *dogstatsd_url = ddtrace_dogstatsd_url();
+        dogstatsd_endpoint = ddog_endpoint_from_url((ddog_CharSlice) {.ptr = dogstatsd_url, .len = strlen(dogstatsd_url)});
+        free(dogstatsd_url);
    }


Here, and this also applies to ddtrace_endpoint, I believe we don't respect the value of DD_SITE. Most specifically, I think that ddog_endpoint_from_api_key doesn't take this env var into consideration.

Maybe ddog_endpoint_from_api_key_and_site should be used? Maybe a new function needs to be created for ddog_endpoint_from_url?

Correct me if I'm wrong

That's correct, however we don't even read that config at all currently. We should support it for sure when we add official agentless support.

Hummmm, I'm thinking of multiple things atm:

For instance, when using Docker, customers will typically only set DD_SITE on the datadog-agent container, not necessarily the instrumented container --> The tracer doesn't know the value of DD_SITE. Is there any communication of this value between the agent and the tracers (not to my knowledge)

(Still using the Docker example) To use the DogStatsD API, does a customer MUST set DD_API_KEY in the instrumented container?

If a European customer wants to use the DogStatsD API, then I believe this... wouldn't work, right?

Nevermind, quickly tested and it seems to work 🤔

I'm not following - the dogstatsd data still goes to the agent - and then agent will then send it to the correct site.

bwoebi

Looks good to me, excellent work :-)

iamluc force-pushed the lv/dogstatsd-api branch 2 times, most recently from 621a6d0 to d7fb685 Compare April 23, 2024 11:29

iamluc force-pushed the lv/dogstatsd-api branch 4 times, most recently from 8a45adc to ea7b71d Compare April 30, 2024 09:47

iamluc marked this pull request as ready for review April 30, 2024 15:00

iamluc requested review from a team as code owners April 30, 2024 15:00

iamluc force-pushed the lv/dogstatsd-api branch 3 times, most recently from 757138f to 051fc5d Compare May 13, 2024 11:53

iamluc mentioned this pull request May 16, 2024

Provide a DogStatsD API in the sidecar DataDog/libdatadog#399

Merged

2 tasks

iamluc added 13 commits May 17, 2024 09:07

Pass the dogstatsd URL to the sidecar

35e38d9

Init the dogstatsd public API

bfbd07a

Add support for tags in dogstatsd functions

623882c

Change the count value type from float to int

65309b0

Add tests

2243053

Use proper Vec<Tag> instead of string serialization

49b63f5

Add support for Distribution and histogram

8edc20a

Fix compilation issues

be120c3

Add support for the 'Set' metric type

1a386f0

Move ddtrace_dogstatsd_url function in a new file as dogstatsd_client…

19a16f8

….c is not compiled on windows

Debug test

1ad19b3

Add global tags

64015e5

Fix handling of invalid tags

e462705

iamluc added 2 commits May 17, 2024 09:17

Fix stubs

311ab94

Skip test failing because of a PHP bug on PHP 7.0/7.1

d7e45dd

iamluc force-pushed the lv/dogstatsd-api branch from 051fc5d to d7e45dd Compare May 17, 2024 07:18

Fix int type: we must support signed numbers

51fbd86

iamluc mentioned this pull request May 22, 2024

Make use of the sidecar thread safe #2671

Closed

2 tasks

PROFeNoM reviewed May 24, 2024

View reviewed changes

bwoebi approved these changes May 24, 2024

View reviewed changes

bwoebi merged commit f246102 into master May 27, 2024

bwoebi deleted the lv/dogstatsd-api branch May 27, 2024 12:39

bwoebi added this to the 1.0.0 milestone May 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a DogStatsD API#2639

Provide a DogStatsD API#2639
bwoebi merged 16 commits intomasterfrom
lv/dogstatsd-api

iamluc commented Apr 23, 2024

Uh oh!

codecov-commenter commented Apr 23, 2024 •

edited

Loading

Uh oh!

pr-commenter Bot commented Apr 25, 2024 •

edited

Loading

Uh oh!

morrisonlevi commented Apr 25, 2024

Uh oh!

PROFeNoM May 24, 2024

Uh oh!

bwoebi May 24, 2024

Uh oh!

PROFeNoM May 24, 2024

Uh oh!

PROFeNoM May 24, 2024

Uh oh!

bwoebi May 24, 2024

Uh oh!

bwoebi left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

iamluc commented Apr 23, 2024

Description

Reviewer checklist

Uh oh!

codecov-commenter commented Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pr-commenter Bot commented Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

scenario:EmptyFileBench/benchEmptyFileBaseline

scenario:EmptyFileBench/benchEmptyFileBaseline-opcache

scenario:EmptyFileBench/benchEmptyFileOverhead

scenario:EmptyFileBench/benchEmptyFileOverhead-opcache

scenario:LaravelBench/benchLaravelBaseline

scenario:LaravelBench/benchLaravelBaseline-opcache

scenario:LaravelBench/benchLaravelOverhead

scenario:LaravelBench/benchLaravelOverhead-opcache

scenario:MessagePackSerializationBench/benchMessagePackSerialization

scenario:MessagePackSerializationBench/benchMessagePackSerialization-opcache

scenario:PDOBench/benchPDOBaseline

scenario:PDOBench/benchPDOOverhead

scenario:PDOBench/benchPDOOverheadWithDBM

scenario:SymfonyBench/benchSymfonyBaseline

scenario:SymfonyBench/benchSymfonyBaseline-opcache

scenario:SymfonyBench/benchSymfonyOverhead

scenario:SymfonyBench/benchSymfonyOverhead-opcache

scenario:WordPressBench/benchWordPressOverhead

scenario:WordPressBench/benchWordPressOverhead-opcache

Uh oh!

morrisonlevi commented Apr 25, 2024

Uh oh!

PROFeNoM May 24, 2024

Choose a reason for hiding this comment

Uh oh!

bwoebi May 24, 2024

Choose a reason for hiding this comment

Uh oh!

PROFeNoM May 24, 2024

Choose a reason for hiding this comment

Uh oh!

PROFeNoM May 24, 2024

Choose a reason for hiding this comment

Uh oh!

bwoebi May 24, 2024

Choose a reason for hiding this comment

Uh oh!

bwoebi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Apr 23, 2024 •

edited

Loading

pr-commenter Bot commented Apr 25, 2024 •

edited

Loading