Skip to content
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.

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

[HWKMETRICS-472] Implement aggregated availability #597

Closed
wants to merge 4 commits into from

Conversation

jotak
Copy link
Contributor

@jotak jotak commented Sep 6, 2016

JIRA: https://issues.jboss.org/browse/HWKMETRICS-472

This PR follows discussion on hawkular-dev (mail title: "Availability metrics: aggregate stats series")

Remark about assertj:
I added assertj, an assertion framework, to the class path in test scope. I've started a topic on hawkular-dev to discuss about assertj. If necessary I will update this PR to remove assertj.

- Collector / aggregator
- Endpoints for availability type
- Unit tests
@rhqci
Copy link

rhqci commented Sep 6, 2016

Can one of the admins verify this patch?

if (limit > 0) {
// Limit already applied on C* query, but might be exceeded after aggregation
// So it's applied again
result = result.limit(limit);
Copy link
Contributor

Choose a reason for hiding this comment

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

.take(limit) is the default in RxJava, limit is an alias (but in ReactiveX world, the normal term is take, so lets use that)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@FilipB
Copy link

FilipB commented Sep 7, 2016

ok to test

List<DataPoint<RatioMap>> result = service.buildRatioMapSeries(allSeries, Order.ASC, AvailabilityType::getText)
.toList()
.toBlocking()
.single();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Reminder: must try with TestSubscriber)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@stefannegrea
Copy link
Contributor

retest this please

@@ -466,4 +471,126 @@ public void getMetricStats(
.map(ApiUtils::collectionToResponse)
.subscribe(asyncResponse::resume, t -> asyncResponse.resume(serverError(t)));
}

@GET
@Path("/aggregated")
Copy link
Contributor

Choose a reason for hiding this comment

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

This path is not good. Aggregated will be used in a different context soon. Can you put this functionality under /stats?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if "stats" fits well. For now "stats" is about time-aggregation, with buckets and so on, whereas here it's about multiple series aggregation, without any time bucket. I fear it would be confusing if we use the same word.

On hawkular-dev ML Jay proposed: "/aggregate, /set, /group or whatever" ... is there anyone of these you find better? Or maybe something like "/merged" ?

}

@POST
@Path("/aggregated/query")
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above about the usage of /aggregated; stats is better.

@stefannegrea
Copy link
Contributor

Just to clarify the comments regarding /stats, we need roll this feature inside stats end-point and provide the ratio map as an optional function the users can select.

@jotak jotak closed this Sep 21, 2016
@jotak jotak deleted the aggregated-availability branch July 28, 2017 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants