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

[tempo-distributed] feat: add cli flag for zone aware ingestion #3230

Merged
merged 7 commits into from
Nov 3, 2024

Conversation

KyriosGN0
Copy link
Contributor

@KyriosGN0 KyriosGN0 commented Jul 14, 2024

like in mimir, we need to pass ingester.ring.instance-availability-zone when enabled zone aware ingestion
FYI @zalegrala

@KyriosGN0 KyriosGN0 changed the title [tempo-dostributed] feat: add cli flag for zone aware ingestion [tempo-distributed] feat: add cli flag for zone aware ingestion Jul 14, 2024
@zalegrala
Copy link
Contributor

I get the following when trying to pass that flag.

flag provided but not defined: -ingester.ring.instance-availability-zone

@KyriosGN0
Copy link
Contributor Author

@zalegrala weird, in our mimir we use this flag, but according to the Cortex Config it should be -ingester.availability-zone
i'll try to compile in jsonnet since it also has a Zone Aware config

@KyriosGN0
Copy link
Contributor Author

hm @zalegrala i looked into the jsonnet and they only set the env var availability_zone which corresponds to the -ingester.availability-zone cli flag according to the cortex docs, so i think its good enough.

@zalegrala
Copy link
Contributor

Each database exposes different config flags, so we can't assume that because its exposed in mimir that it would be exposed in tempo. Have you had a chance to test this change?

@KyriosGN0
Copy link
Contributor Author

@zalegrala -ingester.availability-zone is equivalent to the env var set in the jsonnet file of Tempo, i'll try to deploy this locally in some KIND cluster to see that it recognizes the flag.

@KyriosGN0 KyriosGN0 marked this pull request as draft July 17, 2024 18:11
@KyriosGN0
Copy link
Contributor Author

@zalegrala after taking another look in the jsonnet file i've realized i've missed other things in the chart (like services)
i would also need to open PR to tempo itself to support this cli flag (or figure out how to implement this with the env va)
will ping you when im ready, until then i've converted this to a Draft pr

@KyriosGN0 KyriosGN0 marked this pull request as ready for review October 19, 2024 16:22
@KyriosGN0
Copy link
Contributor Author

@zalegrala should be good now.

@zanhsieh zanhsieh merged commit cc5b880 into grafana:main Nov 3, 2024
6 checks passed
@KyriosGN0 KyriosGN0 deleted the feat-add-zwr-tempo-cli branch November 3, 2024 05:55
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