-
Notifications
You must be signed in to change notification settings - Fork 5.5k
feat(native): Add exchange client runtime stats #26575
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
base: master
Are you sure you want to change the base?
Conversation
kewang1024
commented
Nov 10, 2025
Reviewer's GuideThis PR expands PrestoExchangeSource to collect and expose three new runtime metrics—getData latency, getDataSize latency, and page size—by adding dedicated metric fields, updating processDataResponse to record values, and modifying the metrics() method to include them when available. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- The metric names like "prestoExchangeSource.getDataMs" suggest milliseconds but you're storing nanosecond values (kNanos) — consider renaming them to "getDataNs" or converting the values to ms to avoid confusion.
- You only include the new metrics in the map when count > 0; consider always emitting them (with zero values) so downstream consumers can rely on a consistent metric schema.
- Instead of multiplying durationMs by 1'000'000 to convert to nanoseconds, consider using std::chrono::duration_cast for more precise and robust unit conversion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The metric names like "prestoExchangeSource.getDataMs" suggest milliseconds but you're storing nanosecond values (kNanos) — consider renaming them to "getDataNs" or converting the values to ms to avoid confusion.
- You only include the new metrics in the map when count > 0; consider always emitting them (with zero values) so downstream consumers can rely on a consistent metric schema.
- Instead of multiplying durationMs by 1'000'000 to convert to nanoseconds, consider using std::chrono::duration_cast for more precise and robust unit conversion.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
fdeb702 to
a26e723
Compare
| std::unique_ptr<http::HttpResponse> response, | ||
| bool isGetDataSizeRequest) { | ||
| if (isGetDataSizeRequest) { | ||
| getDataSizeMetric_.addValue( |
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.
The max will always be 10s (equal to the long pool duration). We need to substract the time request is waiting for long pool to make it meaningful.
670089f to
28cae2b
Compare
28cae2b to
84017a9
Compare