-
Notifications
You must be signed in to change notification settings - Fork 805
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
Added metrics for when we rate limit #5640
Conversation
Pull Request Test Coverage Report for Build 018d6eb9-8f0c-484f-8138-3f55479d8974
💛 - Coveralls |
Log spam: eh, it's probably fine tbh. Zap samples by default, per log message (the first string arg). And if we decide we do need something fancier or more control beyond that, we have plenty of options. (I believe we have this disabled internally, but we should probably rethink that. and a log per rpc is far from a major increase anyway.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor tweaks worth doing I think, but merge when ready :)
- Added internal/external to the metrics and logs - Renamed emitMetrics to emitRateLimitMetrics - Added domainID to log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny thing to fix in the tests, but LGTM once that unblocks :)
What changed?
Add metrics and logs for when we rate limit workflowIDs
Why?
We need to have observability on the workflow ids we rate limit
How did you test it?
Tested locally and in unit tests
Potential risks
It might cause excessive logging, but it is at most one log (extra) per GRPC call, so I expect it's ok
Release notes
Documentation Changes