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

Minor metrics / log improvement for global ratelimiter #6259

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

Groxx
Copy link
Contributor

@Groxx Groxx commented Aug 29, 2024

Ran into some issues building a dashboard with these, as the collecton's worker_domain_name value does not automatically relate to everything else's domain-name value, and addressing that is... sadly difficult in our monitoring systems.

Regardless, I think using local keys is probably better. We already have the collection name in logs and metrics, and this way you can search for the domain name and find all of these at once (easier for browsing randomly) and it's no harder to narrow down either (just also search by collection).

Ran into some issues building a dashboard with these, as "worker_domain_name" does not automatically relate to "domain-name", and addressing that is... sadly difficult in our monitoring systems.
Regardless, I think using local keys is probably better.  We already have the collection name in logs and metrics, and this way you can search for the domain name and find all of these at once (easier for browsing randomly) and it's no harder to narrow down either (just also search by collection).
Copy link

codecov bot commented Aug 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.02%. Comparing base (1b02d78) to head (6528b3a).
Report is 1 commits behind head on master.

Additional details and impacted files
Files with missing lines Coverage Δ
common/quotas/global/collection/collection.go 85.02% <100.00%> (ø)

... and 4 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b02d78...6528b3a. Read the comment docs.

func GlobalRatelimiterKeyTag(value string) Tag {
return simpleMetric{key: globalRatelimitKey, value: sanitizer.Value(value)}
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure I understand the problem with the tag, sorry? is it just that this makes joining with other metrics using domain_name hard?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to pull from (and paraphrase) chat:

I'm trying to build this data into our client dashboard, where we have a $domain variable with domain-name in it, which is used for all of our other domain-related queries.
Right now this is emitting collection_name_domain_name, which needs pretty complex rewrites / set intersections / etc to correlate with the domain-name value. Possible, but a bit absurd.

@davidporter-id-au
Copy link
Contributor

discussed offline: tl;dr yes to my question

@Groxx Groxx merged commit 4b76e5b into cadence-workflow:master Aug 29, 2024
20 checks passed
@Groxx Groxx deleted the met-fix branch August 29, 2024 20:51
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.

2 participants