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

Align usage report config with Grafana #5406

Merged
merged 6 commits into from
Mar 2, 2022

Conversation

cyriltovena
Copy link
Contributor

Signed-off-by: Cyril Tovena cyril.tovena@gmail.com

What this PR does / why we need it:

This PR update the config flag to use something similar as Grafana, this way users will be able to find the config to disable more easily.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

cc @KMiller-Grafana let's me know what you want to change on the doc side.

Checklist

  • Documentation added
  • Tests updated
  • Add an entry in the CHANGELOG.md about the changes.

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
@pull-request-size pull-request-size bot added size/M and removed size/S labels Feb 16, 2022
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

LGTM

docs/sources/configuration/_index.md Outdated Show resolved Hide resolved
pkg/usagestats/reporter.go Outdated Show resolved Hide resolved
@@ -261,7 +261,7 @@ func New(cfg Config) (*Loki, error) {
Cfg: cfg,
clientMetrics: chunk_storage.NewClientMetrics(),
}

usagestats.Edition("oss")
Copy link
Member

Choose a reason for hiding this comment

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

nit: I feel this would be better in RegisterFlags

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 plan to overrides this in enterprise and I need a way to know when it happens hence why I move it inside the New here. So I can override after New.

cyriltovena and others added 2 commits February 21, 2022 11:07
Co-authored-by: Christian Haudum <christian.haudum@gmail.com>
Co-authored-by: Christian Haudum <christian.haudum@gmail.com>
Copy link
Contributor

@kavirajk kavirajk left a comment

Choose a reason for hiding this comment

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

LGTM

@kavirajk
Copy link
Contributor

/cc @KMiller-Grafana can you take a look on the doc side?

docs/sources/configuration/_index.md Outdated Show resolved Hide resolved
docs/sources/configuration/_index.md Outdated Show resolved Hide resolved
cyriltovena and others added 2 commits February 24, 2022 20:59
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
@cyriltovena
Copy link
Contributor Author

@KMiller-Grafana let me know if we're good to merge this one, thanks !

Copy link
Contributor

@KMiller-Grafana KMiller-Grafana left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for making these changes.

@cyriltovena cyriltovena merged commit 95fefa6 into grafana:main Mar 2, 2022
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.

6 participants