Add serverStats to BrokerQueryEventListener#14807
Add serverStats to BrokerQueryEventListener#14807cypherean wants to merge 2 commits intoapache:masterfrom
Conversation
| } | ||
|
|
||
| @Override | ||
| public Map<String, Map<String, Integer>> getServerStatsMap() { |
There was a problem hiding this comment.
can you add UTs for this?
| public Map<String, Map<String, Integer>> getServerStatsMap() { | ||
| return _responseMap.entrySet().stream() | ||
| .collect(Collectors.toMap( | ||
| entry -> entry.getKey().getShortName(), |
There was a problem hiding this comment.
The key here is the host right? is there a way to make it more explicit? Maybe instead of using Map<String, Map<String, Integer>> we can use Map<String, ServerStatsInfo> something and use POJO to easily extract the metric values.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14807 +/- ##
============================================
+ Coverage 61.75% 63.82% +2.06%
- Complexity 207 1604 +1397
============================================
Files 2436 2708 +272
Lines 133233 151128 +17895
Branches 20636 23322 +2686
============================================
+ Hits 82274 96452 +14178
- Misses 44911 47454 +2543
- Partials 6048 7222 +1174
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
cypherean
force-pushed
the
shreyaa/serverstats
branch
from
1eadda0 to
037b726
Compare
| package org.apache.pinot.spi.trace; | ||
|
|
||
| public class ServerStatsInfo { | ||
| private final long _submitRequestTimeMs; |
|
|
||
| @Override | ||
| public void setServerStatsMap(Map<String, ServerStatsInfo> serverStatsMap) { | ||
| _serverStatsMap.putAll(serverStatsMap); |
There was a problem hiding this comment.
this is counter-intuitive. Instead of set you can do addToServerStatsMap?
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| package org.apache.pinot.spi.trace; |
There was a problem hiding this comment.
If we are putting this in the SPI we have to take care of a few things.
- A Public ctor with AllArgs is not going to be tenable, since in the future we may want to add more properties to this. Suggest using NoArgs ctor with setters that return
this, or builder pattern. We can also check what convention Pinot uses right now. - We should define precisely each of the fields here. You can add a javadoc for each field member var.
- Could we get rid of redundancies like
ServerStatsinSingleConnectionBrokerRequestHandler? If this is in the SPI would make sense to make this the broadly used class for handling this info.
| _deserializationTimeMs = deserializationTimeMs; | ||
| } | ||
|
|
||
| public long getSubmitRequestTimeMs() { |
There was a problem hiding this comment.
MSE also tracks a bunch of server/stage level stats albeit in a different form. I think we should extend this so it can accept Multistage Query stats too. (I meant that we incorporate it somehow.. not necessarily through the same POJO)
cc: @gortiz who worked on adding MultiStageQueryStats.
There was a problem hiding this comment.
That would be great, but unless I'm wrong, all these trace options only support SSE. It is unclear to me how this is being used, and AFAIK StarTree doesn't use this API. RequestContext itself seems to be SSE focused given it assumes a single table is involved in the query.
|
I don't have enough context. What is BrokerQueryEventListener used for? |
|
We use this within LinkedIn. Essentially our internal piece of code (which links with OSS libraries) publishes metrics to a kafka topic (at a query level with sampling). We get the stats for the completed request from RequestContext and then emit into the topic AFAIK. Before we introduce ServerStatsInfo, can we please take a look at existing classes ServerRoutingStatsEntry and ServerRoutingStatsManager and try to remove duplicate code / make things more consistent. Also, I am not sure why does this need to go into SPI |
|
@siddharthteotia this was added in pinot-spi so that it can be re-used in processing events from event listener. |
|
I think it might make sense for us to share a short proposal. @cypherean and I will get back soon. |
6 participants
Adds server stats to BrokerQueryEventListener, which are published on query completion.
Tested locally: