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

Metric attributes need sort with keys only #1718

Merged
merged 3 commits into from
May 8, 2024

Conversation

cijothomas
Copy link
Member

HashKeyValue is a simple wrapper around KeyValue to provide hash, eq, ord implementations.
The Ord implementation is required to ensure that attributes are treated the same irrespective of the order in which they are passed. This only requires a sort with the Key alone.

This PR simplifies the Ord implementation to be based of the Key part alone from the KeyValue. Also added tests to validate that all measurements aggregate to the same point, irrespective of the order in which KeyValue pairs are passed.

(This is part of a somewhat big refactor of Metrics SDK to significantly improve performance - offering short PRs to make reviews easy. Also intentionally keeping the tests modelled like integration tests, instead of unit tests - this is to reduce churn when refactoring, as lot of internals are expected to change, but the end user facing API need to be relatively stable, as we move towards beta release.)

@cijothomas cijothomas requested a review from a team May 7, 2024 06:19
// "multi_thread" tokio flavor must be used else flush won't
// be able to make progress!
#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
async fn counter_aggregation_attribute_order() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a test to ensure that the dedupe + sort is case-sensitive as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

coming next PR

assert_eq!(
sum.data_points.len(),
1,
"Expected only one data point as attributes are same, but just reordered."
Copy link
Contributor

Choose a reason for hiding this comment

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

We could probably add an assertion to verify that the attributes of the datapoint match what we expect.

@cijothomas cijothomas merged commit 348ec9e into open-telemetry:main May 8, 2024
18 checks passed
@cijothomas cijothomas deleted the cijothomas/simplify-sort branch May 8, 2024 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants