added error count of failed requests from query service to pinot #138
Conversation
| initMetrics(); | ||
| } | ||
|
|
||
| private static final String COUNTER_NAME = "hypertrace.query-service.response.errors"; |
There was a problem hiding this comment.
Move these declarations to the top of the class. And move the inner class defn down. Can you check if the formatter does this automatically?
Codecov Report
@@ Coverage Diff @@
## main #138 +/- ##
============================================
+ Coverage 81.90% 82.00% +0.09%
- Complexity 609 611 +2
============================================
Files 62 62
Lines 2327 2339 +12
Branches 240 240
============================================
+ Hits 1906 1918 +12
Misses 326 326
Partials 95 95
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
adding some further commits based on the feedback will reopen the PR later. |
|
|
||
| private Counter serviceResponseErrorCounter; | ||
| private Counter serviceResponseSuccessCounter; | ||
| private static final String ERROR_COUNTER_NAME = "hypertrace.query-service.response.errors"; |
There was a problem hiding this comment.
See the comment on your other PR - https://github.com/hypertrace/gateway-service/pull/128/files#r854786951
Shall we go with?
hypertrace.query.service.request.status {"error"="true"}
hypertrace.query.service.request.status {"error"="false"}
There was a problem hiding this comment.
Yes, making this change.
| private static final String ERROR_COUNTER_NAME = "hypertrace.query-service.response.errors"; | ||
| private static final String SUCCESS_COUNTER_NAME = "hypertrace.query-service.response.success"; | ||
|
|
||
| class QueryServiceObserver<T> extends ServerCallStreamRxObserver<T> { |
There was a problem hiding this comment.
Can you move this to a separate file?
Would it make sense to move org.hypertrace.core.grpcutils.server.rx of repo https://github.com/hypertrace/java-grpc-utils/tree/main/grpc-server-rx-utils? Something like as below
class ServerCallStreamRxObserverWihMetricRecorder<T> extends ServerCallStreamRxObserver<T> ?
cc: @aaron-steinfeld
There was a problem hiding this comment.
If it were me, I probably wouldn't use the subscription to do this - you only get one (or in some cases, none, if you're not at a terminal op). Extending classes, and introducing classes that themselves may be extended, inevitably leads to more coupling. Instead, taking functional programming to heart, I would prefer composition here. That is, using either doOnError + doOnNext, or doOnEach (which can accept its own observer, or individual callbacks).
In short, this is an exercise in SRP - we can use two individual observers to accomplish the same thing in a clearer and more maintainable way.
There was a problem hiding this comment.
hmmmm... Okay, I got your point. Made the change.
Can you check again? Let me know if it's fine.
aaron-steinfeld
left a comment
aaron-steinfeld
left a comment
There was a problem hiding this comment.
lgtm. Will let @kotharironak approve to ensure it's in line with the other metric change.
|
@Harnoor-se7en Merging this, will cut the release after fixing snyk issue - https://github.com/hypertrace/query-service/runs/6129549813?check_suite_focus=true |
4 participants
Description
Currently, there are no metrics to track the number of failed requests in the data query layer. Adding an error counter to track requests failing in query service. The metric is exposed on /metrics endpoint.
For gateway-service - https://github.com/hypertrace/gateway-service/pull/128