Skip to content

Conversation

@trevorwhitney
Copy link
Collaborator

@trevorwhitney trevorwhitney commented Dec 8, 2025

What this PR does / why we need it:

This PR is to fix the missing goldfish sampling we're seeing in the query-tee. Before using a comparator at sample time, we had a result comparison calculate in the database storage layer. #20004 added better comparisons at sample time, but this tolerance-aware result comparison was not being passed down to database storage. As a result, we were make payload persistence decisions based on the sample time comparison result (and choosing not to persist payload because the results, while a mismatched hash, were within tolerance), but then recording these samples as mismatches in the database.

This PR gets rid of the DB specific result status calculation and passed the comparison outcome to the persistence layer.

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
    • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

@trevorwhitney trevorwhitney force-pushed the twhitney/fix-goldfish-result-comparison branch from cf9591a to 852a405 Compare December 9, 2025 13:58
@trevorwhitney trevorwhitney changed the title Twhitney/fix goldfish result comparison fix: persist correct goldfish result comparison in database Dec 9, 2025
@pull-request-size pull-request-size bot added size/L and removed size/M labels Dec 9, 2025
@trevorwhitney trevorwhitney marked this pull request as ready for review December 9, 2025 20:42
@trevorwhitney trevorwhitney requested a review from a team as a code owner December 9, 2025 20:42
Copilot AI review requested due to automatic review settings December 9, 2025 20:42
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a bug in the goldfish query sampling system where the tolerance-aware comparison result from the comparator was not being persisted to the database. Previously, the database storage layer recalculated the comparison status using only status codes and response hashes, ignoring the tolerance-based matching performed by the comparator. This led to mismatches being recorded in the database even when responses were within acceptable tolerance, resulting in inaccurate goldfish sampling metrics.

Key Changes:

  • Updated the Storage.StoreQuerySample interface to accept a ComparisonResult parameter containing the tolerance-aware comparison status
  • Removed the redundant computeComparisonStatus function from MySQL storage that was recalculating comparison status without tolerance awareness
  • Added logger parameter to CaptureResponse to improve error logging consistency

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
pkg/goldfish/storage.go Updated Storage interface to accept ComparisonResult parameter in StoreQuerySample method
pkg/goldfish/storage_mysql.go Removed computeComparisonStatus function and updated StoreQuerySample to use passed ComparisonResult
pkg/goldfish/storage_noop.go Updated NoopStorage mock to match new interface signature
tools/querytee/goldfish/manager.go Updated processQueryPair to pass ComparisonResult to storage layer; added logger parameter to CaptureResponse
tools/querytee/goldfish/manager_test.go Added test to verify comparison status from comparator is correctly stored; updated mock implementations
tools/querytee/proxy_endpoint_test.go Updated mockGoldfishStorage to match new interface signature
pkg/ui/goldfish_test.go Updated mockStorage to match new interface signature

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@trevorwhitney trevorwhitney merged commit 43a3f15 into main Dec 9, 2025
66 checks passed
@trevorwhitney trevorwhitney deleted the twhitney/fix-goldfish-result-comparison branch December 9, 2025 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants