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: allow using the consistent naming with the signoz collector #4865

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

git-hulk
Copy link
Contributor

Summary

Allow using the consistent ClickHouse cluster name in the collector and query service.

Related Issues / PR's

This resolves SigNoz/signoz-otel-collector#308

@git-hulk git-hulk requested a review from YounixM as a code owner April 15, 2024 03:30
@git-hulk git-hulk changed the title Allow using the consistent naming with the signoz collector feat: allow using the consistent naming with the signoz collector Apr 15, 2024
@git-hulk git-hulk force-pushed the add-cluster-name-flag branch 2 times, most recently from 7f95aab to 2d6bd8a Compare April 15, 2024 03:35
@git-hulk
Copy link
Contributor Author

@srikanthccv I didn't remove the old flag --cluster to avoid breaking the old users, maybe we can remove this in the future after changing all dependencies like the helm charts, and so on.

@@ -52,6 +52,8 @@ func main() {
flag.StringVar(&cacheConfigPath, "experimental.cache-config", "", "(cache config to use)")
flag.StringVar(&fluxInterval, "flux-interval", "5m", "(cache config to use)")
flag.StringVar(&cluster, "cluster", "cluster", "(cluster name - defaults to 'cluster')")
// Allow using the consistent naming with the signoz collector
flag.StringVar(&cluster, "cluster-name", "cluster", "(cluster name - defaults to 'cluster')")
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this override the --cluster always i.e even if someone provided --cluster=my-cluster the value would be default cluster from --cluster-name arg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srikanthccv No, it won't be overridden unless the --cluster-name is provided since the flag parse was only invoked while the flag name was passed. That said, if users specify:

  1. --cluster ABC, then the cluster will be ABC
  2. --cluster-name DEF, then the cluster will be DEF
  3. --cluster ABC --cluster-name DEF, then the cluster will be DEF
  4. --cluster-name DEF --cluster ABC, then the cluster will be ABC

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.

Use --cluster instead of --cluster-name in migrator command flag
2 participants