-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add DogStatsD Package #606
Conversation
This reverts commit 7dc9799.
Currently the sidecar crate is using cadence for its dogstatsd needs. Maybe, as a first use case, and to show the crate working properly, the sidecar might be switched over to this crate? |
@bwoebi The original motivation for this is to use this dogstatsd crate in the serverless so I've done some initial testing to confirm that it works there. But the sidecar can definitely be switched over to use this crate as well! I should have this PR ready to review soon once I get CI passing. |
Ah, I'm seeing you're using 1.77.2. We're currently locked in php to use at most rust 1.76.0 until the next alpine release. |
c8a3b98
to
14241de
Compare
14241de
to
ddb3fc0
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #606 +/- ##
==========================================
+ Coverage 72.69% 73.18% +0.48%
==========================================
Files 246 252 +6
Lines 34990 35959 +969
==========================================
+ Hits 25437 26317 +880
- Misses 9553 9642 +89
|
I'd like to avoid bumping the rust version but I'm running into these errors on versions below
Update: I forked saluki into https://github.com/DataDog/saluki-backport and replaced |
|
||
[dependencies] | ||
datadog-protos = { version = "0.1.0", default-features = false, git = "https://github.com/DataDog/saluki-backport/" } | ||
ddsketch-agent = { version = "0.1.0", default-features = false, git = "https://github.com/DataDog/saluki-backport/" } |
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.
We already have a ddsketch implementation in libdatadog, there is one in mp-rs and there is one on crates.io that is not owned by DD. We should probably all agree on a single implementation.
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.
Interesting, this package uses the same implementation used by the Datadog Lambda Extension (owned by the Serverless team). Since I intend to use this DogStatsD package for a Serverless solution in Azure I'm inclined to stay consistent with the implementations.
https://github.com/DataDog/datadog-lambda-extension/tree/main/bottlecap/src/metrics
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 tried to remove the duplication but I read that this one is slightly different and optimized for the agent. So it might be not possible
https://github.com/DataDog/saluki/blob/main/lib/ddsketch-agent/src/lib.rs#L149
/// This implementation is subtly different from the open-source implementations of
DDSketch
, as Datadog made some
/// slight tweaks to configuration values and in-memory layout to optimize it for insertion performance within the
/// agent.
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.
slight tweaks to configuration values and in-memory layout to optimize it for insertion performance within the agent.
I'm not familiar with sketches, but optimizing in-memory layout and optimizing for insertion performance also sound relevant to a library, if it is feasible. Any insight as to whether it's appropriate or not?
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 am afraid I did not understand your question, can you elaborate more?
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 the original author of the ported DDSketch implementation being used here. As the doc comments allude to, the main goal of the port was to maintain fidelity with the Agent's own DDSketch implementation, which at the time (and to this day) is not based on the open-source Go implementation of DDSketch.
I didn't write the original Agent-specific implementation, so I can't speak to all of the decisions made there, but I also tried to port over some of the code comments to, again, maintain fidelity with the Agent implementation.
While it probably behooves us, in a general sense, to have One True Implementation in Rust... maintaining compatibility with the Datadog Agent in the most important factor in this case.
be204a6
to
ddac4ee
Compare
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.
Does this spawn any threads on the client side of things? I see a test server which probably does, and that's okay. I scanned through it, but it's a big PR so I may have missed something.
Most languages need to have control over threads so they can mask off appropriate signals, register them with the runtime, etc, so the libraries generally need to design APIs that don't spawn threads.
@morrisonlevi Correct, the only thread spawned is in the test. libdatadog/dogstatsd/tests/integration_test.rs Lines 83 to 85 in 5d181a0
|
What does this PR do?
Adds the package
dogstatsd
, a DogStatsD server which uses Saluki for distribution metrics.Moved upstream from https://github.com/DataDog/datadog-lambda-extension/tree/main/bottlecap/src/metrics
Motivation
Add a DogStatsD server that will allow the Serverless Mini Agent to send runtime and custom metrics from Azure Spring Apps and Azure Functions (to be done in an upcoming PR).
https://datadoghq.atlassian.net/browse/SVLS-5177
Additional Notes
1.71.1
to1.76.0
-> changes moved to Upgrade to Rust 1.76.0 #612error[E0445]: crate-private trait MapFieldAccessor in public interface
on1.71.1
,1.72.0
,1.73.0
error[E0658]: use of unstable library feature 'round_ties_even'
on1.74.0
,1.75.0
,1.76.0
round_ties_even()
withround()
reqwest 0.12.4
reqwest 0.12.5
=>no process-level CryptoProvider available -- call CryptoProvider::install_default() before this point
lint
,test
, andmiri
workflowsbash
shell when running test commands soPATH
is picked up properly in Windowsdogstatsd
added to Alpine Dockerfile3.19.3
to work with Rust1.76.0
invalid_reference_casting
instead ofclippy::cast_ref_to_mut
How to test the change?