-
Notifications
You must be signed in to change notification settings - Fork 210
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
[DGS-18780] Ensure p99 metrics are correctly published #519
Conversation
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.
LGTM
).define( | ||
PERCENTILE_MAX_LATENCY_MS_CONFIG, | ||
Type.DOUBLE, | ||
PERCENTILE_MAX_LATENCY_MS_DEFAULT, |
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.
[minor] we should have put the validation here to avoid non positive value
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.
Sure, let me get back with a fix for this.
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.
thanks @pritishatx
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.
Hi @trnguyencflt, raised a PR to add the validation.
We noticed that the percentile latency metrics in rest-utils (request-latency-95 and request-latency-99) are capped out at 10s (code ref).
The PR makes PERCENTILE_MAX_LATENCY_IN_MS configurable in RestConfig, with 10s as the default value.
Slack thread