Add statsd to k8s-run #374
Conversation
lib/kubernetes-deploy/runner_task.rb
Outdated
| @@ -44,11 +45,14 @@ def run!(task_template:, entrypoint:, args:, env_vars: [], verify_result: true) | |||
| else | |||
| record_status_once(pod) | |||
| end | |||
| ::StatsD.distribution('run.duration', StatsD.duration(start), tags: tags('success')) | |||
There was a problem hiding this comment.
why distribution instead of measure?
There was a problem hiding this comment.
Good question, I only learned this when policial yelled at me. I believe its because https://github.com/Shopify/statsd-instrument#statsddistribution is more efficient (/cost effective)
There was a problem hiding this comment.
Hm, those docs say This is only supported by the beta datadog implementatation.--is that going to work for everyone? What was the Policial error exactly? Reading the DD docs about this, I don't really see why Distribution would be the right fit for this kind of metric. These will be really sparse, and we can't do any sampling of them. If we do need to switch, then shouldn't we switch all of our equivalent metrics too?
There was a problem hiding this comment.
Shopify/Datadog: The measure and histogram APIs are no longer the accepted way to submit metrics to Datadog with statsd-instrument. Please prefer StatsD.distribution - a modified guage that submits a distribution of values of a sample period. All arithmetic and calculation happens server side and saves Shopify tons of money on Datadog metric submission counts. See https://github.com/Shopify/statsd-instrument#statsddistribution and https://www.datadoghq.com/blog/summary-graphs-metric-graphs-101/#distributions
There was a problem hiding this comment.
Hm, those docs say This is only supported by the beta datadog implementatation.--is that going to work for everyone?
I think so, though other people will have to talk to DD to get it turned on while it's in beta.
Reading the DD docs about this, I don't really see why Distribution would be the right fit for this kind of metric. These will be really sparse, and we can't do any sampling of them. If we do need to switch,
I didn't see anything about them being sparse. Can you link to where you saw that? What I read (https://docs.datadoghq.com/graphing/metrics/distributions/) makes it look like you have to be explicit in DD itself about which tags from built-in integrations we care about.
then shouldn't we switch all of our equivalent metrics too?
Probably, there are only 9 of them.
There was a problem hiding this comment.
I think so, though other people will have to talk to DD to get it turned on while it's in beta.
I watched the internal video about distributions, and now don't see any internal reason not to switch. But forcing other companies to enable a beta feature on their Datadog account to be able to emit metrics from our gem feels wrong to me.
There was a problem hiding this comment.
Talked to Peer and distributions are in GA, the docs are just out of date.
There was a problem hiding this comment.
Ah, ok. In that case I'm ok with switching them all. This might still count as a breaking change though, since if I understand correctly it will break dashboards until the distribution is configured in Datadog to produce the aggregations we need (and there's no backfill, so we need to be sure to do that right away).
Update to use distribution Use distributions everywhere
dturn
force-pushed
the
statsd-k8s-run
branch
2 times, most recently
from
3a4a1e1
to
b2e9e4a
Compare
|
I've limited the changes to k8s restart & run. Will do deploy later. Last chance for 👀 |
|
👍 |
Add statsd calls to aid in tracking useage.