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

Add metrics to track controller segment download and upload requests in progress #9258

Merged

Conversation

gviedma
Copy link
Contributor

@gviedma gviedma commented Aug 20, 2022

This change tracks the number of in progress segment download and upload requests currently being processed by the controller via their respective Gauge metrics segmentDownloadsInProgress and segmentUploadsInProgress. These two metrics are useful to track the number of in-flight operations in large clusters, where a large number of concurrent uploads/downloads may bring down a shared controller.

This change also introduces a generic JAX-RS annotation-based mechanism that can be reused to track in-flight request metrics of other types in the future. See InflightRequestMetricsInterceptor.

@gviedma gviedma marked this pull request as draft August 20, 2022 21:20
@gviedma gviedma changed the title Collect in progress segment download and upload gauges for controller Track in progress segment download and upload requests Aug 20, 2022
@gviedma gviedma force-pushed the track_controller_inflight_downloads_uploads branch 2 times, most recently from b3d67d5 to 166f5bb Compare August 20, 2022 21:38
@gviedma gviedma changed the title Track in progress segment download and upload requests Add metrics to track in progress segment download and upload requests Aug 20, 2022
@gviedma gviedma changed the title Add metrics to track in progress segment download and upload requests Add metrics to track controller in progress segment download and upload requests Aug 20, 2022
@gviedma gviedma changed the title Add metrics to track controller in progress segment download and upload requests Add metrics to track controller segment download and upload requests in progress Aug 20, 2022
@gviedma gviedma marked this pull request as ready for review August 20, 2022 21:59
@gviedma gviedma force-pushed the track_controller_inflight_downloads_uploads branch 2 times, most recently from ddab727 to 7c2ae1a Compare August 20, 2022 22:17
@codecov-commenter
Copy link

codecov-commenter commented Aug 20, 2022

Codecov Report

Merging #9258 (e5faf6f) into master (718f41f) will increase coverage by 1.20%.
The diff coverage is 88.88%.

@@             Coverage Diff              @@
##             master    #9258      +/-   ##
============================================
+ Coverage     68.75%   69.96%   +1.20%     
- Complexity     4755     5011     +256     
============================================
  Files          1859     1860       +1     
  Lines         99129    99152      +23     
  Branches      15077    15080       +3     
============================================
+ Hits          68161    69371    +1210     
+ Misses        26076    24864    -1212     
- Partials       4892     4917      +25     
Flag Coverage Δ
integration1 26.30% <66.66%> (-0.12%) ⬇️
integration2 24.95% <66.66%> (?)
unittests1 67.12% <0.00%> (+0.04%) ⬆️
unittests2 15.36% <72.22%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...er/api/resources/LLCSegmentCompletionHandlers.java 62.37% <ø> (ø)
...ces/PinotSegmentUploadDownloadRestletResource.java 55.00% <ø> (-0.84%) ⬇️
...i/resources/InflightRequestMetricsInterceptor.java 86.66% <86.66%> (ø)
...g/apache/pinot/common/metrics/ControllerGauge.java 97.91% <100.00%> (+0.09%) ⬆️
...g/apache/pinot/server/api/resources/ErrorInfo.java 0.00% <0.00%> (-100.00%) ⬇️
...t/server/api/resources/DefaultExceptionMapper.java 0.00% <0.00%> (-75.00%) ⬇️
...apache/pinot/broker/api/HttpRequesterIdentity.java 28.57% <0.00%> (-57.15%) ⬇️
...org/apache/pinot/broker/api/RequesterIdentity.java 50.00% <0.00%> (-50.00%) ⬇️
.../pinot/server/api/resources/TableSizeResource.java 80.00% <0.00%> (-8.00%) ⬇️
...nction/DistinctCountBitmapAggregationFunction.java 49.22% <0.00%> (-3.63%) ⬇️
... and 135 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Jackie-Jiang Jackie-Jiang added release-notes Referenced by PRs that need attention when compiling the next release notes observability labels Aug 22, 2022
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@mcvsubbu mcvsubbu left a comment

Choose a reason for hiding this comment

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

So, we should also add the realtime segment upload to the metric.

Done in the class: LLCSegmentCompletionHandlers

This class is invoked by the servers to complete a realtime segment

MISSING_CONSUMING_SEGMENT_MAX_DURATION_MINUTES("missingSegmentsMaxDurationInMinutes", false),

// Number of in progress segment downloads
CONTROLLER_SEGMENT_DOWNLOADS_IN_PROGRESS_COUNT("segmentDownloadsInProgressCount", true),
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe remove CONTROLLER_ prefix? It is in ControllerGauge, emitted by the controller, so we are fine to remove the prefix? Also, maybe remove the _COUNT suffix also?

Metric names are getting long....

Copy link
Contributor Author

@gviedma gviedma Aug 22, 2022

Choose a reason for hiding this comment

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

Good point, I've dropped both the CONTROLLER_ prefix and the _COUNT suffix for readability.
I have also tagged the segmentUpload method in LLCSegmentCompletionHandlers for tracking.

@gviedma gviedma force-pushed the track_controller_inflight_downloads_uploads branch from 7c2ae1a to 50b6063 Compare August 22, 2022 21:42
@gviedma gviedma requested a review from mcvsubbu August 22, 2022 22:38
@mcvsubbu mcvsubbu merged commit fd5c942 into apache:master Aug 23, 2022
@gviedma gviedma deleted the track_controller_inflight_downloads_uploads branch August 23, 2022 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
observability release-notes Referenced by PRs that need attention when compiling the next release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants