feat: Integrate prometheus converter, client and response builder #119
Conversation
This comment has been minimized.
This comment has been minimized.
| Function function = expression.getFunction(); | ||
| String columnName = | ||
| QueryRequestUtil.getLogicalColumnNameForSimpleColumnExpression( | ||
| function.getArguments(0)); |
There was a problem hiding this comment.
Can we have a common function for forming logical name <function_name>:<attributeId>?
|
@rish691 Can you also add a description for this PR? |
This comment has been minimized.
This comment has been minimized.
| this.name = name; | ||
| this.processConfig(config); | ||
| this.queryRequestEligibilityValidator = | ||
| new QueryRequestEligibilityValidator(prometheusViewDefinition); | ||
| this.requestToPromqlConverter = new QueryRequestToPromqlConverter(prometheusViewDefinition); | ||
| String[] hostPort = clientConfig.split(":"); | ||
| this.prometheusRestClient = |
There was a problem hiding this comment.
Can we have a singleton or have a factory way of constructing it? We can use the same RestClient across all the instance of Request Handler.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| } | ||
|
|
||
| PrometheusBasedRequestHandler( | ||
| String name, Config config, PrometheusRestClient prometheusRestClient) { |
There was a problem hiding this comment.
Is this requestHanlder config? If, yes, can we rename to requestHandlerConfig?
| this.prometheusRestClient = PrometheusRestClientFactory.get().getPrometheusClient(name); | ||
| } | ||
|
|
||
| PrometheusBasedRequestHandler( |
There was a problem hiding this comment.
Can we mark this it to only for test or something?
There was a problem hiding this comment.
Can we remove this constructor?
| // Validate QueryContext and tenant id presence | ||
| Preconditions.checkNotNull(executionContext); | ||
| Preconditions.checkNotNull(executionContext.getTenantId()); | ||
|
|
||
| // todo call convert and execute request using client here | ||
| Map<Request, PromQLMetricResponse> responseMap; | ||
| Map<String, String> metricNameToQueryMap = new LinkedHashMap<>(); |
There was a problem hiding this comment.
metricNameToQueryMap -> logicalAttributeNameToMetricQueryMap?
|
|
||
| return null; | ||
| private LinkedHashSet<String> prepareColumnSet( |
There was a problem hiding this comment.
prepareColumnSet-> prepareSelectionColumnSet
| return null; | ||
| private LinkedHashSet<String> prepareColumnSet( | ||
| LinkedHashSet<Expression> expressions, ExecutionContext executionContext) { | ||
| LinkedHashSet<String> columnSet = new LinkedHashSet<>(); |
There was a problem hiding this comment.
columnSet -> selectionColumnSet
|
|
||
| // Create a Prometheus Client. | ||
| public static PrometheusRestClient createPrometheusClient( | ||
| String configName, String connectionString) { |
There was a problem hiding this comment.
configName -> requestHandlerName?
| QueryTimeRange queryTimeRange = getQueryTimeRange(executionContext); | ||
| return new PromQLInstantQueries( | ||
|
|
||
| metricNameToQueryMap.putAll( |
There was a problem hiding this comment.
metricNameToQueryMap -> logicalAttributeNameToMetricQueryMap
This comment has been minimized.
This comment has been minimized.
...rc/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java
Show resolved
Hide resolved
...ce-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusRestClient.java
Show resolved
Hide resolved
| class PrometheusRestClientFactory { | ||
|
|
||
| // Singleton instance | ||
| private static final PrometheusRestClientFactory INSTANCE = new PrometheusRestClientFactory(); |
There was a problem hiding this comment.
Aren't we already using guice?
There was a problem hiding this comment.
Tried using it, isn't looking very clean
There was a problem hiding this comment.
Hmm - IMO, the current form where we implement a singleton by hand, use statics, call a static method and ignore its return to modify state and then needlessly take in a handler name is significantly harder to understand and objectively more code. There's potential bugs in there too, because if I give a handler name that's already been registered, my connection string is ignored.
diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java
index 9f76633..355bbf3 100644
--- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java
+++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java
@@ -36,13 +36,14 @@ public class PrometheusBasedRequestHandler implements RequestHandler {
private Optional<String> startTimeAttributeName;
private PrometheusViewDefinition prometheusViewDefinition;
- PrometheusBasedRequestHandler(String name, Config requestHandlerConfig) {
+ PrometheusBasedRequestHandler(
+ String name, Config requestHandlerConfig, PrometheusRestClient prometheusRestClient) {
this.name = name;
this.processConfig(requestHandlerConfig);
this.queryRequestEligibilityValidator =
new QueryRequestEligibilityValidator(prometheusViewDefinition);
this.requestToPromqlConverter = new QueryRequestToPromqlConverter(prometheusViewDefinition);
- this.prometheusRestClient = PrometheusRestClientFactory.get().getPrometheusClient(name);
+ this.prometheusRestClient = prometheusRestClient;
}
@Override
diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusRequestHandlerBuilder.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusRequestHandlerBuilder.java
index 4136015..acd0be7 100644
--- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusRequestHandlerBuilder.java
+++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusRequestHandlerBuilder.java
@@ -10,10 +10,14 @@ import org.hypertrace.core.query.service.RequestHandlerClientConfigRegistry;
public class PrometheusRequestHandlerBuilder implements RequestHandlerBuilder {
private final RequestHandlerClientConfigRegistry clientConfigRegistry;
+ private final PrometheusRestClientFactory prometheusRestClientFactory;
@Inject
- PrometheusRequestHandlerBuilder(RequestHandlerClientConfigRegistry clientConfigRegistry) {
+ PrometheusRequestHandlerBuilder(
+ RequestHandlerClientConfigRegistry clientConfigRegistry,
+ PrometheusRestClientFactory prometheusRestClientFactory) {
this.clientConfigRegistry = clientConfigRegistry;
+ this.prometheusRestClientFactory = prometheusRestClientFactory;
}
@Override
@@ -32,9 +36,9 @@ public class PrometheusRequestHandlerBuilder implements RequestHandlerBuilder {
new UnsupportedOperationException(
"Client config requested but not registered: " + config.getClientConfig()));
- PrometheusRestClientFactory.createPrometheusClient(
- config.getName(), clientConfig.getConnectionString());
-
- return new PrometheusBasedRequestHandler(config.getName(), config.getRequestHandlerInfo());
+ return new PrometheusBasedRequestHandler(
+ config.getName(),
+ config.getRequestHandlerInfo(),
+ prometheusRestClientFactory.getPrometheusClient(clientConfig.getConnectionString()));
}
}
diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusRestClientFactory.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusRestClientFactory.java
index a1b7bad..e1c8d60 100644
--- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusRestClientFactory.java
+++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusRestClientFactory.java
@@ -1,43 +1,15 @@
package org.hypertrace.core.query.service.prometheus;
import java.util.concurrent.ConcurrentHashMap;
+import javax.inject.Singleton;
+@Singleton
class PrometheusRestClientFactory {
- // Singleton instance
- private static final PrometheusRestClientFactory INSTANCE = new PrometheusRestClientFactory();
-
private final ConcurrentHashMap<String, PrometheusRestClient> clientMap =
new ConcurrentHashMap<>();
- private PrometheusRestClientFactory() {}
-
- // Create a Prometheus Client.
- public static PrometheusRestClient createPrometheusClient(
- String requestHandlerName, String connectionString) {
- if (!get().containsClient(requestHandlerName)) {
- synchronized (get()) {
- if (!get().containsClient(requestHandlerName)) {
- get().addPrometheusClient(requestHandlerName, new PrometheusRestClient(connectionString));
- }
- }
- }
- return get().getPrometheusClient(requestHandlerName);
- }
-
- public static PrometheusRestClientFactory get() {
- return INSTANCE;
- }
-
- private void addPrometheusClient(String requestHandlerName, PrometheusRestClient client) {
- this.clientMap.put(requestHandlerName, client);
- }
-
- public boolean containsClient(String requestHandlerName) {
- return this.clientMap.containsKey(requestHandlerName);
- }
-
- public PrometheusRestClient getPrometheusClient(String requestHandlerName) {
- return this.clientMap.get(requestHandlerName);
+ public PrometheusRestClient getPrometheusClient(String connectionString) {
+ return clientMap.computeIfAbsent(connectionString, PrometheusRestClient::new);
}
}
diff --git a/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandlerTest.java b/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandlerTest.java
index 26a4607..b4d4c09 100644
--- a/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandlerTest.java
+++ b/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandlerTest.java
@@ -40,11 +40,12 @@ class PrometheusBasedRequestHandlerTest {
mockWebServer.start(9099);
Config config = PrometheusTestUtils.getDefaultPrometheusConfig();
- PrometheusRestClientFactory.createPrometheusClient(
- config.getString(CONFIG_PATH_NAME), config.getString(CONFIG_PATH_CLIENT_KEY));
+
prometheusBasedRequestHandler =
new PrometheusBasedRequestHandler(
- config.getString(CONFIG_PATH_NAME), config.getConfig(CONFIG_PATH_REQUEST_HANDLER_INFO));
+ config.getString(CONFIG_PATH_NAME),
+ config.getConfig(CONFIG_PATH_REQUEST_HANDLER_INFO),
+ new PrometheusRestClient(config.getString(CONFIG_PATH_CLIENT_KEY)));
}
@AfterAll| return null; | ||
| private LinkedHashSet<String> prepareSelectionColumnSet( | ||
| LinkedHashSet<Expression> expressions, ExecutionContext executionContext) { | ||
| LinkedHashSet<String> selectionColumnSet = new LinkedHashSet<>(); |
There was a problem hiding this comment.
collect and make immutable rather than returning an immutable type. Also, why are we returning LinkedHashSet If we're concerned about order and need to declare it, return a list; if we're not concerned about order or we are in this function but not elsewhere, return a Set. In other words, does this api need to both declare that it's unique and ordered, or are those implementation details?
There was a problem hiding this comment.
The requirement from this method is that the returned collection has unique element and the order is same as the input collection.
Now returning set from this method would mean losing the order.
A list can be returned as it will maintain the order (the input collection already has unique elements so will the list)
If you see the flow of data,
this method is invoked on a collection in the ExecutionContext class, which returns object of type LinkedHashSet,
then that collection is processed by this method. Finally it is passed into the response builder class a method param.
PrometheusBasedResponseBuilder.buildResponse(
responseMap,
prometheusViewDefinition.getAttributeMap(),
logicalAttributeNameToMetricQueryMap,
prepareSelectionColumnSet(executionContext.getAllSelections(), executionContext),
executionContext.getTimeFilterColumn());
Checking the users of executionContext.getAllSelections(), I think this collection itself could have been of List type
I understand that wherever possible we should return interface, but how to convey that the param LinkedHashSet<String> columnSelectionSet (which will be changed to List<String> columnSelectionSet) in this method org.hypertrace.core.query.service.prometheus.PrometheusBasedResponseBuilder#buildResponse must have ordered and unique elements?
There was a problem hiding this comment.
I understand that wherever possible we should return interface, but how to convey that the param LinkedHashSet columnSelectionSet (which will be changed to List columnSelectionSet) in this method org.hypertrace.core.query.service.prometheus.PrometheusBasedResponseBuilder#buildResponse must have ordered and unique elements?
The problem here isn't just "use an interface instead of an implementation", it's what that rule of thumb is based on - that returning the type is leaking implementation details. It's much less about the implementation of the collection itself, it's that we're trying to convey the responsibilities of one class to another with it. We only need (or want) one class to understand how to prepare selections, and that's the only place that needs to know if duplicates are allowed or not. The caller can then trust that prepare the selection column set was responsible for doing everything involved in preparing a selection set, but it doesn't need to know what that was.
| String columnName = | ||
| QueryRequestUtil.getLogicalColumnNameForSimpleColumnExpression( | ||
| functionExpression.getFunction().getArguments(0)); | ||
| return String.join(":", functionName, columnName); |
There was a problem hiding this comment.
This makes me nervous - a lot of the work on the attribute expressions feature has been in fixing similar code where we generate aliases or use attribute IDs as column names in the result metadata and ignore the requested alias. Since this is ignoring the requested alias, is it for internal use only or does this show up in the response?
There was a problem hiding this comment.
It's for internal mapping and internal use only (It doesn't show up in response). As we have to fire multiple queries for the same QS query, and if that query has two metrics function on the same attributed, we needed a unique mapping.
e.g
select SERVICE.service_id, sum(SERVICE.numCalls), avg(SERVICE.numCalls).
As in the above example, the same attribute id is used for two different metric functions. It wasn't the issue in the case of PinotBase response where it can be mapped using column index. But, here, we will have two PromQL responses - one for sum(SERVICE.numCalls) and one for avg(SERVICE.numCalls). So, internally, we need a unique map of the response, so we are using <funcname>:<attributeId> internally for response building.
There was a problem hiding this comment.
Got it; In other places, we were building the response metadata like this, rather than using the alias. As long as it's switching back to that before returning, we're good.
Update PrometheusBasedRequestHandler to convert queryRequest to Prometheus query, invoke the rest client to fire the query and finally convert the response to query response