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.

Sign up for GitHub

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

Jump to bottom

[tempo-distributed] Add support for querier.config.search.external_backend #2553

Merged
merged 1 commit into from
Jul 31, 2023
Merged

[tempo-distributed] Add support for querier.config.search.external_backend #2553

merged 1 commit into from
Jul 31, 2023

Conversation

modulitos
Copy link
Contributor

@modulitos modulitos commented Jul 27, 2023 •
edited

This PR does the following:
1. Updates the tempo-distributed helm chart to the latest v2.2 Tempo release
2. Removes the now-incompatible tolerate_failed_blocks config, as announced in the Tempo v2.2.x release: https://github.com/grafana/tempo/releases/tag/v2.2.0-rc.0
3. Adds support for the external_backend and google_cloud_run configs, to leverage the new RBAC feature

I tested a local build of this helm chart, and can confirm that it works correctly with the new 2.2 release.

Checklist:

  • DCO signed
  • Chart Version bumped
  • Title of the PR starts with chart name (e.g. [tempo-distributed])

modulitos
modulitos commented Jul 27, 2023
View reviewed changes
charts/tempo-distributed/README.md Outdated
@@ -1,6 +1,6 @@
# tempo-distributed

![Version: 1.4.13](https://img.shields.io/badge/Version-1.4.13-informational?style=flat-square) ![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square) ![AppVersion: 2.1.1](https://img.shields.io/badge/AppVersion-2.1.1-informational?style=flat-square)
![Version: 1.5.0](https://img.shields.io/badge/Version-1.5.0-informational?style=flat-square) ![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square) ![AppVersion: 2.2.0-rc.0](https://img.shields.io/badge/AppVersion-2.2.0--rc.0-informational?style=flat-square)
Copy link
Contributor Author

@modulitos modulitos Jul 27, 2023 •
edited

Choose a reason for hiding this comment

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

I only bumped this chart a minor version because that's what we've used previously when removing incompatible configs. Happy to bump it a major version if that's preferred.

For example:

Copy link
Contributor

Choose a reason for hiding this comment

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

1.5.x seems okay to me for the major version, and the backwards incompatibilities. Drawing folks attention to the changelog/readme would be good, and not sure folks look all the time for minor version changes.

Copy link
Contributor Author

@modulitos modulitos Jul 28, 2023 •
edited

Choose a reason for hiding this comment

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

I added this section to the readme, which links to the relevant portions of the Tempo changelog:

## From Chart versions < 1.5.0

Please be aware that we've updated the minor version to Tempo 2.2, which includes breaking changes.
We recommend reviewing the [release notes](https://github.com/grafana/tempo/releases/tag/v2.2.0-rc.0/) before upgrading.

Lmk if there's something else I should add or change 👍

@modulitos modulitos force-pushed the tempo-distributed-2.2 branch 3 times, most recently from 6ae8d5c to 8c9248d Compare July 27, 2023 22:03
zalegrala
zalegrala reviewed Jul 28, 2023
View reviewed changes
charts/tempo-distributed/Chart.yaml Outdated
version: 1.4.13
appVersion: 2.1.1
version: 1.5.0
appVersion: 2.2.0-rc.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want the RC? I think if we wait a week or so, we'll have a proper release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I was a little eager to try out the new RC 😁

Happy to wait for the official release, or do a separate PR for it - either is fine with me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think wait maybe two days and we'll have a release. But by all means, test out the image with these changes if you can.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/grafana/tempo/releases/tag/v2.2.0 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated 🎉

Copy link
Contributor

Choose a reason for hiding this comment

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

@joe-elliott put up #2562 for review. See some slight differences between the two.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this PR is a superset of Joe's. Both PRs upgrade to 2.2.0 and remove the deprecated tolerate_failed_blocks, but this PR also adds support for the new querier.config.search.external_backend feature.

Copy link
Contributor Author

@modulitos modulitos Jul 31, 2023 •
edited

Choose a reason for hiding this comment

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

Happy to followup with adding querier.config.search.external_backend in a followup PR, or we can merge this one to have it all done at once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zalegrala I rebased this PR so that it only updates querier.config.search.external_backend, now that the v2.2.0 changes have been merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. Thanks for the updates.

@modulitos modulitos force-pushed the tempo-distributed-2.2 branch 2 times, most recently from 844a5a9 to 3ed1495 Compare July 31, 2023 19:51
@modulitos modulitos changed the title [tempo-distributed] Use tempo 2.2, remove queryFrontend.config.tolerate_failed_blocks [tempo-distributed] Add support for querier.config.search.external_backend Jul 31, 2023
@modulitos
Support cloud_run configs
d0fac6e 
Signed-off-by: modulitos <hi@modulitos.com>
@zalegrala zalegrala merged commit 9236b1e into grafana:main Jul 31, 2023
7 checks passed
@modulitos modulitos mentioned this pull request Jul 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zalegrala zalegrala zalegrala approved these changes

@annanay25 annanay25 Awaiting requested review from annanay25

@joe-elliott joe-elliott Awaiting requested review from joe-elliott

@mapno mapno Awaiting requested review from mapno mapno is a code owner

@mdisibio mdisibio Awaiting requested review from mdisibio

@swartz-k swartz-k Awaiting requested review from swartz-k swartz-k is a code owner

@BitProcessor BitProcessor Awaiting requested review from BitProcessor BitProcessor is a code owner

@faustodavid faustodavid Awaiting requested review from faustodavid faustodavid is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@modulitos @zalegrala