You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "soumitra-st (via GitHub)" <gi...@apache.org> on 2024/05/17 22:45:19 UTC
[PR] Improved metrics for server grpc query [pinot]
soumitra-st opened a new pull request, #13177:
URL: https://github.com/apache/pinot/pull/13177
Added transport filter to keep track of number of gRPC connections, and added metrics to keep track of total execution times of successful and failed queries.
`dependencies`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Improved metrics for server grpc query [pinot]
Posted by "soumitra-st (via GitHub)" <gi...@apache.org>.
soumitra-st commented on code in PR #13177:
URL: https://github.com/apache/pinot/pull/13177#discussion_r1607368116
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/grpc/GrpcQueryServer.java:
##########
@@ -72,19 +77,38 @@ public class GrpcQueryServer extends PinotQueryServerGrpc.PinotQueryServerImplBa
private final AccessControl _accessControl;
private final ServerQueryLogger _queryLogger = ServerQueryLogger.getInstance();
+ // Filter to keep track of gRPC connections.
+ private class GrpcQueryTransportFilter extends ServerTransportFilter {
+ @Override
+ public Attributes transportReady(Attributes transportAttrs) {
+ LOGGER.info("gRPC transportReady: REMOTE_ADDR {}",
Review Comment:
Proxy creates the transport when it gets the first query for a server, and does not close it, so no risk of flooding the log file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Improved metrics for server grpc query [pinot]
Posted by "soumitra-st (via GitHub)" <gi...@apache.org>.
soumitra-st commented on code in PR #13177:
URL: https://github.com/apache/pinot/pull/13177#discussion_r1607476930
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/grpc/GrpcQueryServer.java:
##########
@@ -162,13 +187,16 @@ public void submit(ServerRequest request, StreamObserver<ServerResponse> respons
_serverMetrics.addMeteredGlobalValue(ServerMeter.NO_TABLE_ACCESS, 1);
responseObserver.onError(
Status.NOT_FOUND.withDescription(exceptionMsg).withCause(unsupportedOperationException).asException());
+ return;
Review Comment:
Request should terminate after onError, so added return.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Improved metrics for server grpc query [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #13177:
URL: https://github.com/apache/pinot/pull/13177
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Improved metrics for server grpc query [pinot]
Posted by "soumitra-st (via GitHub)" <gi...@apache.org>.
soumitra-st commented on code in PR #13177:
URL: https://github.com/apache/pinot/pull/13177#discussion_r1608544667
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerTimer.java:
##########
@@ -53,6 +53,8 @@ public enum ServerTimer implements AbstractMetrics.Timer {
"Total time taken to preload a table partition of an upsert table with upsert snapshot"),
UPSERT_REMOVE_EXPIRED_PRIMARY_KEYS_TIME_MS("milliseconds", false,
"Total time taken to delete expired primary keys based on metadataTTL or deletedKeysTTL"),
+ GRPC_QUERY_EXECUTION_MS("milliseconds", true, "Total execution time of a successful query over gRPC"),
+ GRPC_FAILED_QUERY_EXECUTION_MS("milliseconds", true, "Total execution time of a failing query over gRPC"),
Review Comment:
Removed it.
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerTimer.java:
##########
@@ -53,6 +53,8 @@ public enum ServerTimer implements AbstractMetrics.Timer {
"Total time taken to preload a table partition of an upsert table with upsert snapshot"),
UPSERT_REMOVE_EXPIRED_PRIMARY_KEYS_TIME_MS("milliseconds", false,
"Total time taken to delete expired primary keys based on metadataTTL or deletedKeysTTL"),
+ GRPC_QUERY_EXECUTION_MS("milliseconds", true, "Total execution time of a successful query over gRPC"),
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Improved metrics for server grpc query [pinot]
Posted by "soumitra-st (via GitHub)" <gi...@apache.org>.
soumitra-st commented on code in PR #13177:
URL: https://github.com/apache/pinot/pull/13177#discussion_r1608543693
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/grpc/GrpcQueryServer.java:
##########
@@ -72,19 +77,38 @@ public class GrpcQueryServer extends PinotQueryServerGrpc.PinotQueryServerImplBa
private final AccessControl _accessControl;
private final ServerQueryLogger _queryLogger = ServerQueryLogger.getInstance();
+ // Filter to keep track of gRPC connections.
+ private class GrpcQueryTransportFilter extends ServerTransportFilter {
+ @Override
+ public Attributes transportReady(Attributes transportAttrs) {
+ LOGGER.info("gRPC transportReady: REMOTE_ADDR {}",
Review Comment:
This is also logging the remote address to help in troubleshooting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Improved metrics for server grpc query [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #13177:
URL: https://github.com/apache/pinot/pull/13177#discussion_r1607298327
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/grpc/GrpcResultsBlockStreamer.java:
##########
@@ -44,6 +48,8 @@ public void send(BaseResultsBlock block)
Collection<Object[]> rows = block.getRows();
Preconditions.checkState(dataSchema != null && rows != null, "Malformed data block");
DataTable dataTable = block.getDataTable();
- _streamObserver.onNext(StreamingResponseUtils.getDataResponse(dataTable));
+ Server.ServerResponse response = StreamingResponseUtils.getDataResponse(dataTable);
+ _streamObserver.onNext(response);
+ _serverMetrics.addMeteredGlobalValue(ServerMeter.GRPC_BYTES_SENT, response.getSerializedSize());
Review Comment:
Good catch on this bug. Can you also add this fix into the PR description?
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerTimer.java:
##########
@@ -53,6 +53,8 @@ public enum ServerTimer implements AbstractMetrics.Timer {
"Total time taken to preload a table partition of an upsert table with upsert snapshot"),
UPSERT_REMOVE_EXPIRED_PRIMARY_KEYS_TIME_MS("milliseconds", false,
"Total time taken to delete expired primary keys based on metadataTTL or deletedKeysTTL"),
+ GRPC_QUERY_EXECUTION_MS("milliseconds", true, "Total execution time of a successful query over gRPC"),
+ GRPC_FAILED_QUERY_EXECUTION_MS("milliseconds", true, "Total execution time of a failing query over gRPC"),
Review Comment:
Failed query execution time might not be very useful
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerTimer.java:
##########
@@ -53,6 +53,8 @@ public enum ServerTimer implements AbstractMetrics.Timer {
"Total time taken to preload a table partition of an upsert table with upsert snapshot"),
UPSERT_REMOVE_EXPIRED_PRIMARY_KEYS_TIME_MS("milliseconds", false,
"Total time taken to delete expired primary keys based on metadataTTL or deletedKeysTTL"),
+ GRPC_QUERY_EXECUTION_MS("milliseconds", true, "Total execution time of a successful query over gRPC"),
Review Comment:
Let's make this table level
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/grpc/GrpcQueryServer.java:
##########
@@ -72,19 +77,38 @@ public class GrpcQueryServer extends PinotQueryServerGrpc.PinotQueryServerImplBa
private final AccessControl _accessControl;
private final ServerQueryLogger _queryLogger = ServerQueryLogger.getInstance();
+ // Filter to keep track of gRPC connections.
+ private class GrpcQueryTransportFilter extends ServerTransportFilter {
+ @Override
+ public Attributes transportReady(Attributes transportAttrs) {
+ LOGGER.info("gRPC transportReady: REMOTE_ADDR {}",
Review Comment:
Will this potentially flood the log? Since we already have the metrics, maybe no need to log it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Improved metrics for server grpc query [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #13177:
URL: https://github.com/apache/pinot/pull/13177#issuecomment-2118482537
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/13177?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `0%` with `25 lines` in your changes are missing coverage. Please review.
> Project coverage is 0.00%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`b2506ec`)](https://app.codecov.io/gh/apache/pinot/pull/13177?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 460 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/13177?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...che/pinot/core/transport/grpc/GrpcQueryServer.java](https://app.codecov.io/gh/apache/pinot/pull/13177?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Ftransport%2Fgrpc%2FGrpcQueryServer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS90cmFuc3BvcnQvZ3JwYy9HcnBjUXVlcnlTZXJ2ZXIuamF2YQ==) | 0.00% | [16 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13177?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../core/transport/grpc/GrpcResultsBlockStreamer.java](https://app.codecov.io/gh/apache/pinot/pull/13177?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Ftransport%2Fgrpc%2FGrpcResultsBlockStreamer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS90cmFuc3BvcnQvZ3JwYy9HcnBjUmVzdWx0c0Jsb2NrU3RyZWFtZXIuamF2YQ==) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13177?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...a/org/apache/pinot/common/metrics/ServerMeter.java](https://app.codecov.io/gh/apache/pinot/pull/13177?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Fmetrics%2FServerMeter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9TZXJ2ZXJNZXRlci5qYXZh) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13177?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...a/org/apache/pinot/common/metrics/ServerTimer.java](https://app.codecov.io/gh/apache/pinot/pull/13177?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Fmetrics%2FServerTimer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9TZXJ2ZXJUaW1lci5qYXZh) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13177?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #13177 +/- ##
=============================================
- Coverage 61.75% 0.00% -61.76%
=============================================
Files 2436 2445 +9
Lines 133233 134657 +1424
Branches 20636 20841 +205
=============================================
- Hits 82274 0 -82274
- Misses 44911 134657 +89746
+ Partials 6048 0 -6048
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/13177/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/13177/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/13177/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/13177/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/13177/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/13177/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/13177/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.63%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/13177/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.75%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/13177/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/13177/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.76%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/13177/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/13177/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/13177/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/13177?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Improved metrics for server grpc query [pinot]
Posted by "soumitra-st (via GitHub)" <gi...@apache.org>.
soumitra-st commented on code in PR #13177:
URL: https://github.com/apache/pinot/pull/13177#discussion_r1608544108
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/grpc/GrpcResultsBlockStreamer.java:
##########
@@ -44,6 +48,8 @@ public void send(BaseResultsBlock block)
Collection<Object[]> rows = block.getRows();
Preconditions.checkState(dataSchema != null && rows != null, "Malformed data block");
DataTable dataTable = block.getDataTable();
- _streamObserver.onNext(StreamingResponseUtils.getDataResponse(dataTable));
+ Server.ServerResponse response = StreamingResponseUtils.getDataResponse(dataTable);
+ _streamObserver.onNext(response);
+ _serverMetrics.addMeteredGlobalValue(ServerMeter.GRPC_BYTES_SENT, response.getSerializedSize());
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org