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

[chore] Method that sorts Datapoint Slice by Attribute key #23068

Merged

Conversation

BominRahmani
Copy link
Contributor

Adds a method that allows Metrics Datapoint slices of Golden files to be sorted by their respective attribute key. This is in the efforts to bring Golden files 1 step closer to being more deterministic.

There is a test folder (sort-datapoint-slice) in golden/testdata that shows an example of what this accomplishes.

@BominRahmani BominRahmani marked this pull request as ready for review June 6, 2023 14:23
@BominRahmani BominRahmani requested review from a team June 6, 2023 14:23
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

Please also regenerate all golden files so we can see what the expected change is.

internal/coreinternal/golden/sort_metrics.go Outdated Show resolved Hide resolved
internal/coreinternal/golden/sort_metrics.go Show resolved Hide resolved
internal/coreinternal/golden/sort_metrics.go Show resolved Hide resolved
name: nsxt.node.network.io
sum:
aggregationTemporality: 2
dataPoints:
Copy link
Member

Choose a reason for hiding this comment

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

Can we have some additional data points that exercise the sorting of attributes within a map? Further, can we have some that require comparison beyond the first key?

  • a
  • b, a
  • c, b
  • c, b, a
  • c, a, d

Comment on lines 15 to 16
startTimeUnixNano: "1111111111111111111"
timeUnixNano: "1111111111111111111"
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to omit these fields for this test, as they only distract from the point of the test.

@BominRahmani
Copy link
Contributor Author

Please also regenerate all golden files so we can see what the expected change is.

This is already all the changes when running the sort datapoint slice.

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

This is already all the changes when running the sort datapoint slice.

Ok, I suppose this makes sense because we generally want data points to have the same attribute keys.

Since this PR doesn't improve the golden files, I think it makes sense to extend it to cover resources and scopes, where I expect we'll see meaningful improvements.

For data points, I think a followup PR will make sense, which layers on sorting by value, since the values tend to be static or even enumerated.

@djaglowski
Copy link
Member

Thanks @BominRahmani, @dehaansa!

@djaglowski djaglowski merged commit cab5ab9 into open-telemetry:main Jun 9, 2023
@github-actions github-actions bot added this to the next release milestone Jun 9, 2023
Caleb-Hurshman pushed a commit to observIQ/opentelemetry-collector-contrib that referenced this pull request Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants