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

feat(blooms): ignore individual bloom-gw failures #12863

Merged
merged 4 commits into from
May 2, 2024

Conversation

owen-d
Copy link
Member

@owen-d owen-d commented May 2, 2024

  • includes a new metric to record statuses now that we ignore errors
  • Also removes old/no longer relevant cache locality score metric since we now resolve bloom-gw membership in a different way

Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
@owen-d owen-d requested a review from a team as a code owner May 2, 2024 17:59
"blocks", len(rs.blocks),
"err", err,
)
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

In case a request fails, the results entries need to be equal to the request entries, otherwise we assume that all requested chunks have been filtered out.

Suggested change
return nil
results[i] = rs.groups
count += len(rs.groups)
return nil

Copy link
Contributor

Choose a reason for hiding this comment

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

Increasing count is not strictly needed, but the value is used for the allocation of the result buffer.

Buckets: prometheus.LinearBuckets(0.01, 0.2, 5),
}),
Name: "requests_total",
Help: "Total number of requests made to the bloom gateway",
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
Copy link
Contributor

@chaudum chaudum left a comment

Choose a reason for hiding this comment

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

Minor nitpicks, but approved to unblock

"blocks", len(rs.blocks),
"err", err,
)
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Increasing count is not strictly needed, but the value is used for the allocation of the result buffer.

@@ -52,7 +52,7 @@ func TestGatewayClient_MergeSeries(t *testing.T) {
{Fingerprint: 0x01, Refs: []*logproto.ShortRef{shortRef(0, 1, 3), shortRef(1, 2, 4)}}, // fully overlapping chunks
{Fingerprint: 0x02, Refs: []*logproto.ShortRef{shortRef(0, 1, 5), shortRef(1, 2, 6)}}, // partially overlapping chunks
},
// response 2
// response 3
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

owen-d added 2 commits May 2, 2024 15:09
…in all cases

Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
@owen-d owen-d merged commit 4c9b22f into grafana:main May 2, 2024
59 checks passed
grafanabot pushed a commit that referenced this pull request May 6, 2024
chaudum pushed a commit that referenced this pull request May 6, 2024
Backport 4c9b22f from #12863

Co-authored-by: Owen Diehl <ow.diehl@gmail.com>
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.

None yet

2 participants