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

query-tee: add support for comparing native histograms in results #8724

Merged
merged 10 commits into from
Jul 16, 2024

Conversation

charleskorn
Copy link
Contributor

@charleskorn charleskorn commented Jul 15, 2024

What this PR does

This PR adds support for comparing native histograms in query results to query-tee.

Previously, any native histograms in query results were ignored.

I've also taken this opportunity to clarify some of the error messages generated when results do not match, and to replace the use of errors.Wrap with fmt.Errorf in response_comparator.go.

Which issue(s) this PR fixes or relates to

(none)

Checklist

  • Tests updated.
  • [n/a] Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • [n/a] about-versioning.md updated with experimental features.

@charleskorn charleskorn force-pushed the charleskorn/query-tee-native-histograms branch from 0147308 to 718ca00 Compare July 15, 2024 06:56
@charleskorn charleskorn marked this pull request as ready for review July 15, 2024 07:10
@charleskorn charleskorn requested a review from a team as a code owner July 15, 2024 07:10
Copy link
Contributor

@krajorama krajorama left a comment

Choose a reason for hiding this comment

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

Approved with tiny nit, thank you for the extensive test coverage! According to VS code all branches are covered.

return math.Abs(first-second) <= opts.Tolerance
}

func compareSampleHistogramPair(expected, actual model.SampleHistogramPair, opts SampleComparisonOptions) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: call inputs first and second as compareSampleValue. Function doesn't care and in line with the other function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function does care about which input is which, as they're used to produce the error message when comparison fails (eg. "expected timestamp X but got Y").

The names here mirror those of compareSamplePair, which uses these names for the same reason.

tools/querytee/response_comparator.go Show resolved Hide resolved
@charleskorn charleskorn enabled auto-merge (squash) July 16, 2024 00:34
@charleskorn charleskorn merged commit b8fe2a8 into main Jul 16, 2024
29 checks passed
@charleskorn charleskorn deleted the charleskorn/query-tee-native-histograms branch July 16, 2024 00:46
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.

2 participants