Support passing Unix timestamps to dogstatsd #831
Conversation
carlosroman
force-pushed
the
carlosroman/add-support-for-ts-in-metrics
branch
3 times, most recently
from
cc7e57b
to
717b7c7
Compare
carlosroman
changed the title
| # timestamps (protocol v1.3) only allowed on gauges and counts | ||
| allows_timestamp = metric_type == "g" or metric_type == "c" | ||
| if not allows_timestamp or timestamp < 0: | ||
| timestamp = 0 |
There was a problem hiding this comment.
Do you think that would be worth a warning? It's a developer error at this point (somebody is calling _report with a wrong metric_type)
There was a problem hiding this comment.
Yes worth a test (as said in the other comment) but maybe we don't want to flood stdout with logs?
There was a problem hiding this comment.
I added a test but not sure about adding a warning. In other SDKs we don't log a warning if invalid timestamp used. Here if they are trying to send a metric that doesn't support timestamps then they're accessing methods they shouldn't be in the first place.
| self.statsd.count_with_timestamp("page.views", 1, timestamp=-1066) | ||
| self.statsd.flush() | ||
| self.assert_equal_telemetry("page.views:1|c\n", self.recv(2)) | ||
|
|
There was a problem hiding this comment.
Would that be worth a direct call to _report with a timestamp and a wrong metric type to make sure it sets the timestamp to 0? Not sure.
There was a problem hiding this comment.
Yeah, I feel there should be one more test to make sure that if statement that checks metric types with timestamps is covered correctly.
carlosroman
added
the
changelog/Added
carlosroman
force-pushed
the
carlosroman/add-support-for-ts-in-metrics
branch
from
092bc54
to
c2b4b9b
Compare
carlosroman
force-pushed
the
carlosroman/add-support-for-ts-in-metrics
branch
from
c2b4b9b
to
96c6ddf
Compare
|
/merge |
|
🚂 MergeQueue Pull request added to the queue. This build is going to start soon! (estimated merge in less than 0s) Use |
|
/merge |
|
🚂 MergeQueue Pull request added to the queue. This build is going to start soon! (estimated merge in less than 0s) Use |
dd-devflow
bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: error
mergequeue-status: queued
labels
|
/merge |
|
🚂 MergeQueue Pull request added to the queue. This build is going to start soon! (estimated merge in less than 0s) Use |
|
🚨 MergeQueue There was an unexpected error while creating the working branch This could indicate that something doesn't work properly with the build system or that this one has reached its maximum capacity. Detailschild workflow execution error (type: mergequeue_private.MergeQueue_BuildWorkingBranch, workflowID: c26f1b50-9a2c-493b-aa0d-b5854ec3bdee_39, runID: 4d8f467f-d452-45f7-8a74-4c633fc273c2, initiatedEventID: 39, startedEventID: 40): Child workflow timeout (type: StartToClose) If you need support, contact us on Slack #devflow with those details! |
|
/merge |
|
🚂 MergeQueue Pull request added to the queue. This build is going to start soon! (estimated merge in less than 0s) Use |
|
/merge |
|
🚂 MergeQueue Pull request added to the queue. This build is going to start soon! (estimated merge in less than 0s) Use |
|
🚨 MergeQueue There was an unexpected error while creating the working branch This could indicate that something doesn't work properly with the build system or that this one has reached its maximum capacity. Detailschild workflow execution error (type: mergequeue_private.MergeQueue_BuildWorkingBranch, workflowID: 7c0fa5e8-8ba8-415f-83b2-de8437385968_39, runID: b470ac72-d453-4a4d-bbed-cfa0624e8b2f, initiatedEventID: 39, startedEventID: 40): Child workflow timeout (type: StartToClose) If you need support, contact us on Slack #devflow with those details! |
What does this PR do?
Following v1.3 protocol, allow passing timestamps along with the metric, which can mitigate the load on the agent when emitting a lot of metrics in a short time span.
Description of the Change
Adds
gauge_with_timestampandcount_with_timestampto allow for passing metrics with timestamps. Also addedcountmethod which other clients have so that our clients all have similar methods.Alternate Designs
Possible Drawbacks
Verification Process
Additional Notes
Release Notes
Review checklist (to be filled by reviewers)
changelog/label attached. If applicable it should have thebackward-incompatiblelabel attached.do-not-merge/label attached.kind/andseverity/labels attached at least.