-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
metrics/dogstatsd: always reset all metrics before writing them when calling WriteTo #1231
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
Conversation
peterbourgon
commented
Jun 2, 2022
If local state fails to get sent to Datadog and you reset it anyway, doesn't that lose information and invalidate your metrics?
It does, but it's coherent with the method's comment I suppose: "WriteTo abides best-effort semantics, so observations are
lost if there is a problem with the write.". The current implementation already loses some information if you have multiple counters for example but you fail to send the first one, no?
We could chose to buffer these metrics while the connection is down (i.e.: only resetting when all writes are successful) instead but then we expose ourselves to the ever-growing memory issue if it lasts for too long.
peterbourgon
commented
Jun 3, 2022
That's fair, I overlooked that caveat in the docs.
skwair
commented
Jun 10, 2022
Is it ok for you or should I update the PR?
skwair
commented
Jul 20, 2022
Hello, sorry for bumping this again, but we would need this fix on our side, any chance this gets merged? Or do you see another implementation for fixing this issue?
MaxymVlasov
commented
Aug 1, 2022
@peterbourgon can you please take a look at this? It, as a dependency, affects an app in our production env for two months and a downgrade is not an option.
peterbourgon
commented
Aug 1, 2022
I'm happy to merge with a test that fails on current master and passes on the branch.
bb963bf to
f89002e
Compare
This prevents the Dogstatsd client to consume an ever-increasing amount of memory when it fails to write somme metrics. Without this fix, if counters can't be written, timings and histograms are not reset.
skwair
commented
Aug 2, 2022
Hello, thanks for your reply.
I added a test that fails without the fix as you asked. Let me know if it works for you.
MaxymVlasov
commented
Aug 9, 2022
If I understand correctly, to notify @peterbourgon about comments, need to mention him
skwair
commented
Sep 7, 2022
Hello @peterbourgon, did you have time to check if the tests added match what you were asking for?
ldez
commented
Nov 18, 2022
@peterbourgon @ChrisHines @basvanbeek friendly ping
Description
This PR fixes an issue that occurred when the Dogstatsd client could not reach the Datadog agent to send metrics. It would fail to send counters, return and never reset timings and histograms, resulting in an ever-increasing memory consumption while the client cannot reach the Datadog agent.