Update cassandra jmx metrics script to combine similar metrics into labelled metric

[dehaansa]

Description: Several cassandra metrics are all related counters describing specific operations & statuses of request, and can now be easily grouped together after the recent PR was merged to support lists of mbeans supporting a single metric.

[anuraaga]

This is a bit too intense of an assertion to understand easily. Is it just checking that every point contains every attributeGroup? How about just passing in a map of merged attributes instead of separate groups?

[dehaansa]

This is providing a list of the labelled data points expected, and validating that all of the expected combinations exist. For example, if we expected one data point with { operation: Read, status: Timeout } and one data point with { operation Read, status: Ok }, the attributeGroups parameter would contain a list of those two maps. Are there ways I could make that more clear?

[jsuereth]

I think the complain is on the use of .stream() when you already have varargs to handle it.

E.g. I think you could do:

 assertThat(points)
        .extracting(point -> point.getAttributesList())
        .satisfiesExactlyInAnyOrder(
           list -> assertThat(list).contains(...),
           list -> assertThat(list).contains(...),
        );

Would also help if there was a "assertThatAttributeList(list, KV)" type helper method.

You can look in https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/metrics-testing/src/main/java/io/opentelemetry/sdk/testing/assertj/metrics/MetricAssertions.java for ideas. Might be nice to have a good set of assertion libraries for the OTLP protos...

[anuraaga]

and validating that all of the expected combinations exist

I may not be understanding well but currently think this is the key point. In particular the word "all". If so, it seems the parameter passed to the assertion could have all of the attributes in one Map rather than a list of maps. Is this not the case?

[dehaansa]

Each item in the provided list represents one data point's AttributeList, and this method currently validates that we have N different data points that match N unique sets of AttributeLists, with one data point matching each one unique set of Attributes. I don't see a way to reduce this to a single map.

To expand on the example for this cassandra metric, we have 12 data points under one metric that we are validating, where each data point has a different combination of "operation" and "status" attributes. So the list contains the maps { operation: Read, status: Ok }, { operation: Read, status: Timeout }, { operation: Read, status: Unavailable }, { operation: Read, status: Failure }, { operation: Write, status: Ok }, { operation: Write, status: Timeout }, { operation: Write, status: Unavailable }, { operation: Write, status: Timeout }, { operation: Write, status: Failure }, { operation: RangeSlice, status: Ok },...

[jsuereth]

Change itself looks good. Would be nice to improve readability of the unit test (as Anuraag suggests)

[jsuereth]

I think the complain is on the use of .stream() when you already have varargs to handle it.

E.g. I think you could do:

 assertThat(points)
        .extracting(point -> point.getAttributesList())
        .satisfiesExactlyInAnyOrder(
           list -> assertThat(list).contains(...),
           list -> assertThat(list).contains(...),
        );

Would also help if there was a "assertThatAttributeList(list, KV)" type helper method.

You can look in https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/metrics-testing/src/main/java/io/opentelemetry/sdk/testing/assertj/metrics/MetricAssertions.java for ideas. Might be nice to have a good set of assertion libraries for the OTLP protos...

[rmfitzpatrick]

The existing metrics are documented here and would need updating: https://github.com/open-telemetry/opentelemetry-java-contrib/blob/main/jmx-metrics/docs/target-systems/cassandra.md

[rmfitzpatrick]

@dehaansa this looks great, but would you be able to update the docs as well? Unfortunately they aren't generated.

https://github.com/open-telemetry/opentelemetry-java-contrib/blob/main/jmx-metrics/docs/target-systems/cassandra.md

[dehaansa]

@dehaansa this looks great, but would you be able to update the docs as well? Unfortunately they aren't generated.

https://github.com/open-telemetry/opentelemetry-java-contrib/blob/main/jmx-metrics/docs/target-systems/cassandra.md

Was waiting for final tweaks to look good before updating, will do so shortly.