Skip to content
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

metrics/dogstatsd: always reset all metrics before writing them when calling WriteTo #1231

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

skwair
Copy link

@skwair skwair commented Jun 2, 2022

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.

@peterbourgon
Copy link
Member

If local state fails to get sent to Datadog and you reset it anyway, doesn't that lose information and invalidate your metrics?

@skwair
Copy link
Author

skwair commented Jun 3, 2022

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
Copy link
Member

That's fair, I overlooked that caveat in the docs.

@skwair
Copy link
Author

skwair commented Jun 10, 2022

Is it ok for you or should I update the PR?

@skwair
Copy link
Author

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
Copy link

@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
Copy link
Member

I'm happy to merge with a test that fails on current master and passes on the branch.

@skwair skwair force-pushed the fix-datadog-metrics-memleak branch from bb963bf to f89002e Compare August 2, 2022 09:59
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
Copy link
Author

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
Copy link

If I understand correctly, to notify @peterbourgon about comments, need to mention him

@skwair
Copy link
Author

skwair commented Sep 7, 2022

Hello @peterbourgon, did you have time to check if the tests added match what you were asking for?

@ldez
Copy link
Contributor

ldez commented Nov 18, 2022

@peterbourgon @ChrisHines @basvanbeek friendly ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants