Skip to content

fix: fix bug where expectedStreamUsageRequest was not used #17223

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

Merged
merged 1 commit into from
Apr 15, 2025

Conversation

grobinson-grafana
Copy link
Contributor

@grobinson-grafana grobinson-grafana commented Apr 14, 2025

What this PR does / why we need it:

fix bug where expectedStreamUsageRequest was not used in the tests for the limits-frontend.

Which issue(s) this PR fixes:
Fixes #

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

@grobinson-grafana grobinson-grafana requested a review from a team as a code owner April 14, 2025 22:15
@@ -171,10 +177,10 @@ func TestFrontend_ExceedsLimits(t *testing.T) {
// and instance 1 for the data for partition 1.
expectedStreamUsageRequest: []*logproto.GetStreamUsageRequest{{
Tenant: "test",
StreamHashes: []uint64{0x1, 0x2},
StreamHashes: []uint64{0x2},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the main fix. When we stopped sending the partition ID in GetStreamusageRequest, we started sending just the stream hashes for the owned partitions, instead of all stream hashes. However, because expectedStreamUsageRequest was not being used in the test, this was never asserted. This test should have failed when we made the change a couple weeks ago.

@grobinson-grafana grobinson-grafana changed the title fix: fix bug where expectedStreamusageRequest was not used fix: fix bug where expectedStreamUsageRequest was not used Apr 15, 2025
@grobinson-grafana grobinson-grafana enabled auto-merge (squash) April 15, 2025 10:06
@grobinson-grafana grobinson-grafana merged commit d106042 into main Apr 15, 2025
62 checks passed
@grobinson-grafana grobinson-grafana deleted the grobinson/fix-limits-frontend-tests branch April 15, 2025 10:23
chaudum pushed a commit that referenced this pull request Apr 15, 2025
Contains backports of these commits:

```
2cde9b1 fix: skip streams over limits in dry-run mode (#17114)
805125c fix: fix a bug where limits were incorrect after #16937 (#17224)
d106042 fix: fix bug where expectedStreamUsageRequest was not used (#17223)
69aeda1 feat: add tests for enforcing limits in distributors (#17124)
```
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