Add controller execution time tracking to query responses #17709
Add controller execution time tracking to query responses #17709Akanksha-kedia wants to merge 1 commit intoapache:masterfrom
Conversation
|
curl -X POST http://localhost:9001/sql -H "Content-Type: application/json" -d '{"sql":"SELECT COUNT(*) FROM baseballStats"}' | jq . % Total % Received % Xferd Average Speed Time Time Time Current (#17647 (comment)) |
|
@shauryachats @xiangfu0 @Jackie-Jiang please review |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #17709 +/- ##
============================================
- Coverage 63.24% 63.21% -0.04%
Complexity 1454 1454
============================================
Files 3176 3176
Lines 191002 191017 +15
Branches 29204 29204
============================================
- Hits 120791 120742 -49
- Misses 60796 60862 +66
+ Partials 9415 9413 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@xiangfu0 @shauryachats @Jackie-Jiang please review and merge |
Jackie-Jiang
left a comment
There was a problem hiding this comment.
Having controllerExecutionTimeMs field in BrokerResponse is quite counter intuitive. What is the purpose of this field? To track to additional overhead of controller-broker communication?
|
Yes
…On Wed, 18 Feb 2026 at 2:31 AM, Xiaotian (Jackie) Jiang < ***@***.***> wrote:
***@***.**** commented on this pull request.
Having controllerExecutionTimeMs field in BrokerResponse is quite counter
intuitive. What is the purpose of this field? To track to additional
overhead of controller-broker communication?
—
Reply to this email directly, view it on GitHub
<#17709 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AVL2AZUKKIRENALT4ACSYHL4MN6UFAVCNFSM6AAAAACVIN4RQ6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTQMJWGI4TIMJSHA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Does it work if you add an extra field in the controller response and not modify |
|
+1. @Akanksha-kedia as I mentioned in your previous PR #17647:
Additionally, if this information is only supposed to be bubbled up to the Controller UI, you can instead modify the UI code to expose the actual latency of the controller endpoint. |
d6e986e to
b889e14
Compare
…g BrokerResponse - Add controllerExecutionTimeMs as an additional field in controller SQL query responses - Do not modify BrokerResponseNative to avoid increasing response size for direct broker queries - Keep timeseries responses unchanged by using separate sendTimeSeriesRequestRaw method - Fix GET endpoint method signature to use proper executeTimeSeriesQueryCatching parameters - Ensures controller preserves broker's PinotBrokerTimeSeriesResponse structure with status field This addresses review feedback to only add execution time for controller UI queries, not for all broker queries, by adding the field at the controller API level rather than modifying the shared BrokerResponseNative class.
b889e14 to
32d6387
Compare
…meseries responses
The issue was that controller was incorrectly trying to parse timeseries responses as BrokerResponseNative and add execution time, but timeseries responses use PinotBrokerTimeSeriesResponse with different structure. This fix preserves the original Prometheus-compatible response format from the broker.
Instructions:
featurebugfixperformanceuibackward-incompatrelease-notes(**)(*) Other labels to consider:
testingdependenciesdockerkubernetesobservabilitysecuritycode-styleextension-pointrefactorcleanup(**) Use
release-noteslabel for scenarios like: