Skip to content

added error count of failed requests from query service to pinot #138

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.

Sign up for GitHub

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

Merged
kotharironak merged 4 commits into from
Apr 23, 2022
Merged

added error count of failed requests from query service to pinot #138

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6e3be19
added error count of failed request from query-service to pinot/other…
Harnoor-se7en Apr 7, 2022
84d249e
added success metric for response returned in query-layer
Harnoor-se7en Apr 8, 2022
c0df366
added success metric and applied the spotless github action fix
Harnoor-se7en Apr 8, 2022
2eb6185
addressing the PR comments 1
Harnoor-se7en Apr 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@

import static org.hypertrace.core.query.service.RowChunkingOperator.chunkRows;

import com.google.common.collect.ImmutableMap;
import io.grpc.Status;
import io.grpc.stub.ServerCallStreamObserver;
import io.grpc.stub.StreamObserver;
import io.micrometer.core.instrument.Counter;
import io.reactivex.rxjava3.core.Maybe;
import io.reactivex.rxjava3.core.Observable;
import javax.inject.Inject;
Expand All @@ -16,14 +18,32 @@
import org.hypertrace.core.query.service.api.QueryServiceGrpc;
import org.hypertrace.core.query.service.api.ResultSetChunk;
import org.hypertrace.core.query.service.validation.QueryValidator;
import org.hypertrace.core.serviceframework.metrics.PlatformMetricsRegistry;

@Singleton
@Slf4j
class QueryServiceImpl extends QueryServiceGrpc.QueryServiceImplBase {

private final RequestHandlerSelector handlerSelector;
private final QueryTransformationPipeline queryTransformationPipeline;
private final QueryValidator queryValidator;

private Counter serviceResponseErrorCounter;

class QueryServiceObserver<T> extends ServerCallStreamRxObserver<T> {

@kotharironak kotharironak Apr 21, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@aaron-steinfeld aaron-steinfeld Apr 21, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@Harnoor-se7en Harnoor-se7en Apr 22, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hmmmm... Okay, I got your point. Made the change.
Can you check again? Let me know if it's fine.


public QueryServiceObserver(StreamObserver<T> serverCallStreamObserver) {
super(serverCallStreamObserver);
}

@Override
public void onError(Throwable th) {
serviceResponseErrorCounter.increment();
super.onError(th);
}

}

@Inject
public QueryServiceImpl(
RequestHandlerSelector handlerSelector,
Expand All @@ -32,6 +52,15 @@ public QueryServiceImpl(
this.handlerSelector = handlerSelector;
this.queryTransformationPipeline = queryTransformationPipeline;
this.queryValidator = queryValidator;
initMetrics();
}

private static final String COUNTER_NAME = "hypertrace.query-service.response.errors";

@suddendust suddendust Apr 7, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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?

@Harnoor-se7en Harnoor-se7en Apr 8, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved them.


private void initMetrics() {
serviceResponseErrorCounter =
PlatformMetricsRegistry.registerCounter(
COUNTER_NAME, ImmutableMap.of());
}

@Override
Expand All @@ -47,7 +76,7 @@ public void execute(
originalRequest, requestContext.getTenantId().orElseThrow())))
.doOnError(error -> log.error("Query failed: {}", originalRequest, error))
.subscribe(
new ServerCallStreamRxObserver<>(
new QueryServiceObserver<>(
(ServerCallStreamObserver<ResultSetChunk>) callStreamObserver));
}

Expand Down