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

CI: build with -tags stringlabels #5731

Merged
merged 2 commits into from
Aug 18, 2023
Merged

CI: build with -tags stringlabels #5731

merged 2 commits into from
Aug 18, 2023

Conversation

bboreham
Copy link
Contributor

What this PR does

Change the tags used in CI to include stringlabels.
This uses less memory, especially in ingesters. See #3555.

Checklist

  • Tests updated
  • NA Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

This uses less memory, especially in ingesters.
@pracucci pracucci self-requested a review August 14, 2023 11:47
CHANGELOG.md Outdated
@@ -43,7 +43,7 @@
* [ENHANCEMENT] Distributor: optimize sending of requests to ingesters by reusing memory buffers for marshalling requests. #5195 #5389
* [ENHANCEMENT] Querier: add experimental `-querier.minimize-ingester-requests` option to initially query only the minimum set of ingesters required to reach quorum. #5202 #5259 #5263
* [ENHANCEMENT] Querier: improve error message when streaming chunks from ingesters to queriers and a query limit is reached. #5245
* [ENHANCEMENT] Use new data structure for labels, to reduce memory consumption. #3555
* [ENHANCEMENT] Use new data structure for labels, to reduce memory consumption. #3555, #5731
Copy link
Collaborator

Choose a reason for hiding this comment

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

[nit] We've a script parsing CHANGELOG (used during the release process) and I think doesn't parse the numbers correctly with a ,

Suggested change
* [ENHANCEMENT] Use new data structure for labels, to reduce memory consumption. #3555, #5731
* [ENHANCEMENT] Use new data structure for labels, to reduce memory consumption. #3555 #5731

Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

LGTM. There are docs which mention how to run integration tests. I think it's worth changing the instructions to also include stringlabels so that CI and local integration tests run more or less under the same conditions.

```
go test -v -tags=requires_docker ./integration/...
```
If you want to run a single test you can use a filter. For example, to only run `TestChunksStorageAllIndexBackends`:
```
go test -v -tags=requires_docker ./integration -run "^TestChunksStorageAllIndexBackends$"
```
When running all integration tests, the test process may time out before the tests complete. If this happens, you can increase `go test`'s default 10 minute timeout to something longer with the `-timeout` flag, for example:
```
go test -v -tags=requires_docker -timeout=20m ./integration/...
```

@bboreham
Copy link
Contributor Author

There are docs which mention how to run integration tests. I think it's worth changing the instructions to also include stringlabels so that CI and local integration tests run more or less under the same conditions.

Note it's not too important how the test driver is built; they invoke the image which will have been built with stringlabels.
However I can change the docs if you think consistency is important.

@bboreham
Copy link
Contributor Author

It would also be good to tell people to add -stringlabels to go test, and to their IDE config for running tests, the debugger, etc.
But I'm not aware of any centralised way to do this.

@dimitarvdimitrov
Copy link
Contributor

There are docs which mention how to run integration tests. I think it's worth changing the instructions to also include stringlabels so that CI and local integration tests run more or less under the same conditions.

Note it's not too important how the test driver is built; they invoke the image which will have been built with stringlabels. However I can change the docs if you think consistency is important.

That's a good point. Happy to leave docs out of this PR

@pracucci pracucci merged commit 27f1709 into main Aug 18, 2023
27 checks passed
@pracucci pracucci deleted the default-stringlabels branch August 18, 2023 14:09
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.

3 participants