Provide a DogStatsD API in the sidecar #399
Conversation
iamluc
force-pushed
the
lv/sidecar-dogstatsd-bridge
branch
from
157309c
to
8a53272
Compare
iamluc
force-pushed
the
lv/sidecar-dogstatsd-bridge
branch
2 times, most recently
from
db1fdc9
to
d4507c7
Compare
iamluc
force-pushed
the
lv/sidecar-dogstatsd-bridge
branch
5 times, most recently
from
0a8b35b
to
417dd3f
Compare
iamluc
force-pushed
the
lv/sidecar-dogstatsd-bridge
branch
2 times, most recently
from
a647b7b
to
567bded
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #399 +/- ##
==========================================
- Coverage 66.17% 66.00% -0.17%
==========================================
Files 187 188 +1
Lines 23171 23470 +299
==========================================
+ Hits 15334 15492 +158
- Misses 7837 7978 +141
|
| @@ -334,6 +334,15 @@ dependencies = [ | |||
| "serde", | |||
| ] | |||
|
|
|||
| [[package]] | |||
| name = "cadence" | |||
There was a problem hiding this comment.
I know it's less of a problem in the sidecar than in the library but what's the size impact of this library?
There was a problem hiding this comment.
You mean on libdatadog_profiling.{a,so}? The size is exactly the same
There was a problem hiding this comment.
It won't change the profiling lib. It will change the final package tracer size I assume as the sidecar is used by tracing only for now. Basically, our release artifacts are 500MB already, the profiling team starts looking into reducing package size, we should globally be very careful of the size impact of adding any new dependency.
There was a problem hiding this comment.
The dd-trace-php PR implementing the DogStatsD API generated a package of 521 MB.
The last PR merged on master generated a package of 516 MB.
It's about a 1% increase
There was a problem hiding this comment.
Thanks. We support 12 versions in the package IIRC. So I assume we have an extra 400KB per version here. Not saying it comes from cadence only though. Anyway, this is something that we will need to be more and more careful about I believe even for the sidecar (not a blocker here I suppose).
| instance_id: &InstanceId, | ||
| metric: ffi::CharSlice, | ||
| value: u64, | ||
| tags: Option<&ddcommon_ffi::Vec<Tag>>, |
There was a problem hiding this comment.
So we passa all the tags all the time? Some of them (e.g lang, lang_interpreter, lang_version, tracer_version, runtimeId won't change from one session to another.
Also, not saying we should do it, but we do process at least some tags in .NET (cf this), do you do it on php side?
There was a problem hiding this comment.
Yes, we do that on the PHP side. The language/version/runtime id tags could be probably directly provided within the sidecar. PR #398 anyway transfers that language/version info as part of session info. We could just use that part and merge it here. Should be more efficient too :-)
Would make sense to me, great point, Pierre!
There was a problem hiding this comment.
If processing happens on the client side we should document it Somewhere in the API that we expect input data to be already processed.
iamluc
force-pushed
the
lv/sidecar-dogstatsd-bridge
branch
from
567bded
to
d0c543a
Compare
iamluc
force-pushed
the
lv/sidecar-dogstatsd-bridge
branch
from
d0c543a
to
6bcb3a0
Compare
What does this PR do?
Add a DogStatsD client in the sidecar, and expose an API to send metrics through it.
Motivation
Is part of the PHP tasks for Q2. It adds new possibilities for the integration team.
Additional Notes
The underlying StatsD client used by this PR is Cadence.
It is already used by some teams at Datadog, even if does not complies exactly to other DogStatsD implementations like the one in PHP or Go
For example, it lacks support of the "sample rate", or requires an integer for the
Setmetric, while Go and PHP can accept a string.But I think there is nothing critical that blocks us.
How to test the change?
Describe here in detail how the change can be validated.
For Reviewers
@DataDog/security-design-and-guidance.