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

Support passing Unix timestamps to dogstatsd #831

Merged
merged 4 commits into from
May 23, 2024

Conversation

carlosroman
Copy link
Contributor

@carlosroman carlosroman commented May 3, 2024

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_timestamp and count_with_timestamp to allow for passing metrics with timestamps. Also added count method 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)

  • Feature or bug fix MUST have appropriate tests (unit, integration, etc...)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have one changelog/ label attached. If applicable it should have the backward-incompatible label attached.
  • PR should not have do-not-merge/ label attached.
  • If Applicable, issue must have kind/ and severity/ labels attached at least.

@carlosroman carlosroman force-pushed the carlosroman/add-support-for-ts-in-metrics branch 3 times, most recently from cc7e57b to 717b7c7 Compare May 3, 2024 11:29
@carlosroman carlosroman changed the title Carlosroman/add support for ts in metrics Support passing Unix timestamps to dogstatsd May 3, 2024
remeh
remeh previously approved these changes May 3, 2024
Copy link
Contributor

@remeh remeh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes worth a test (as said in the other comment) but maybe we don't want to flood stdout with logs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 carlosroman added the changelog/Added Added features results into a minor version bump label May 3, 2024
@carlosroman carlosroman force-pushed the carlosroman/add-support-for-ts-in-metrics branch from 092bc54 to c2b4b9b Compare May 3, 2024 15:05
@carlosroman carlosroman marked this pull request as ready for review May 3, 2024 15:55
@carlosroman carlosroman requested review from a team as code owners May 3, 2024 15:55
@carlosroman carlosroman requested a review from remeh May 3, 2024 15:56
@carlosroman carlosroman force-pushed the carlosroman/add-support-for-ts-in-metrics branch from c2b4b9b to 96c6ddf Compare May 7, 2024 10:23
@carlosroman
Copy link
Contributor Author

/merge

@dd-devflow
Copy link

dd-devflow bot commented May 22, 2024

🚂 MergeQueue

Pull request added to the queue.

This build is going to start soon! (estimated merge in less than 0s)

Use /merge -c to cancel this operation!

@dd-devflow
Copy link

dd-devflow bot commented May 22, 2024

🚨 MergeQueue

Gitlab pipeline didn't start on its own and we were unable to create it... Please retry.

If you need support, contact us on Slack #devflow with those details!

@carlosroman
Copy link
Contributor Author

/merge

@dd-devflow
Copy link

dd-devflow bot commented May 23, 2024

🚂 MergeQueue

Pull request added to the queue.

This build is going to start soon! (estimated merge in less than 0s)

Use /merge -c to cancel this operation!

@carlosroman
Copy link
Contributor Author

/merge

@dd-devflow
Copy link

dd-devflow bot commented May 23, 2024

🚂 MergeQueue

Pull request added to the queue.

This build is going to start soon! (estimated merge in less than 0s)

Use /merge -c to cancel this operation!

@dd-devflow
Copy link

dd-devflow bot commented May 23, 2024

🚨 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.
You can try to wait a bit and then re-add your pull request to the queue!

Details

child 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!

@carlosroman
Copy link
Contributor Author

/merge

@dd-devflow
Copy link

dd-devflow bot commented May 23, 2024

🚂 MergeQueue

Pull request added to the queue.

This build is going to start soon! (estimated merge in less than 0s)

Use /merge -c to cancel this operation!

@dd-devflow
Copy link

dd-devflow bot commented May 23, 2024

🚨 MergeQueue

Gitlab pipeline didn't start on its own and we were unable to create it... Please retry.

If you need support, contact us on Slack #devflow with those details!

@carlosroman
Copy link
Contributor Author

/merge

@dd-devflow
Copy link

dd-devflow bot commented May 23, 2024

🚂 MergeQueue

Pull request added to the queue.

This build is going to start soon! (estimated merge in less than 0s)

Use /merge -c to cancel this operation!

@dd-devflow
Copy link

dd-devflow bot commented May 23, 2024

🚨 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.
You can try to wait a bit and then re-add your pull request to the queue!

Details

child 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!

@carlosroman carlosroman merged commit cca8ac7 into master May 23, 2024
45 of 47 checks passed
@carlosroman carlosroman deleted the carlosroman/add-support-for-ts-in-metrics branch May 23, 2024 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/Added Added features results into a minor version bump mergequeue-status: error resource/dogstatsd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants