feat: implement histogram analytics use case #12364
Conversation
dev-marek
force-pushed
the
apim-10031-return-histogram-data
branch
from
5306e5c to
0692ffc
Compare
| @@ -191,6 +194,17 @@ public Optional<TopFailedAggregate> searchTopFailedApis(QueryContext queryContex | |||
| return this.client.search(indexes, null, esQuery).map(SearchTopFailedApisAdapter::adaptResponse).blockingGet(); | |||
| } | |||
|
|
|||
| @Override | |||
| public List<HistogramAggregate<?>> searchHistogram(QueryContext queryContext, HistogramQuery query) { | |||
There was a problem hiding this comment.
WDYT of including a Collection definitionVersions and call the appropriate index? This could help in the future if we start creating histograms from both indexes.
I'll let @Okhelifi weigh in too 👍
| @@ -58,4 +61,6 @@ public interface AnalyticsRepository { | |||
| Optional<TopHitsAggregate> searchTopApps(QueryContext queryContext, TopHitsQueryCriteria criteria); | |||
|
|
|||
| Optional<TopFailedAggregate> searchTopFailedApis(QueryContext queryContext, TopFailedQueryCriteria criteria); | |||
|
|
|||
| List<HistogramAggregate<?>> searchHistogram(QueryContext queryContext, HistogramQuery query); | |||
There was a problem hiding this comment.
Why do you use ? instead of reflecting more what is accepted by a HistogramAggregate T extends Number?
| } | ||
| return aggregations | ||
| .stream() | ||
| .map(a -> new Aggregation(a.getField(), Aggregation.AggregationType.valueOf(a.getType().toUpperCase()))) |
There was a problem hiding this comment.
WDYT about handling if a.getType() is null and if a.getType().toUppercase() is not a given value of Aggregation.AggregationType?
| } | ||
|
|
||
| private void validateApiProxy(Api api) { | ||
| if (api.getApiDefinitionHttpV4().isTcpProxy()) { |
There was a problem hiding this comment.
This will be a NPE if the Api is Native. WDYT of -->
| if (api.getApiDefinitionHttpV4().isTcpProxy()) { | |
| if (api.getApiDefinitionHttpV4() != null && api.getApiDefinitionHttpV4().isTcpProxy()) { |
There was a problem hiding this comment.
The use case mocks all interactions with the services, which I find a bit of a bummer since the onion architecture allows us to test up to the repository level.
@jgiovaresco @Okhelifi Is there an official point of view from the devs about mocks in the onion architecture services?
There was a problem hiding this comment.
I agree we should avoid mocks in UseCase tests. Especially in this case where we have InMemory implementations available for ApiCrudService and AnalyticsQueryService
|
Some tests for the |
Issue
https://gravitee.atlassian.net/browse/APIM-10031
Description
Implement MAPI and logic for histogram analytics for v4 API based on v2 implementation
Additional context
📚 View the storybook of this branch here