-
Notifications
You must be signed in to change notification settings - Fork 151
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
Provide a DogStatsD API #2639
Provide a DogStatsD API #2639
Conversation
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
621a6d0
to
d7fb685
Compare
BenchmarksBenchmark execution time: 2024-05-17 08:02:17 Comparing candidate commit 51fbd86 in PR branch Found 14 performance improvements and 5 performance regressions! Performance is the same for 159 metrics, 0 unstable metrics. 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
|
FYI there is a tiny dogstatsd client that should probably be cleaned up or deleted at |
8a45adc
to
ea7b71d
Compare
757138f
to
051fc5d
Compare
….c is not compiled on windows
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); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, quickly tested and it seems to work 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not following - the dogstatsd data still goes to the agent - and then agent will then send it to the correct site.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, excellent work :-)
Description
Provide a DogStatsD API.
Related libdatadog PR => DataDog/libdatadog#399
Reviewer checklist