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

Add -usage-stats.installation-mode configuration to track the installation mode #3244

Merged
merged 11 commits into from
Oct 20, 2022
Merged

Add -usage-stats.installation-mode configuration to track the installation mode #3244

merged 11 commits into from
Oct 20, 2022

Conversation

hi-rustin
Copy link
Contributor

@hi-rustin hi-rustin commented Oct 18, 2022 •
edited by pracucci
Loading

What this PR does

  • Add -usage-stats.installation-mode configuration to track the installation mode.

Which issue(s) this PR fixes or relates to

Part of #3149

Checklist

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

@aknuds1
Copy link
Contributor

aknuds1 commented Oct 18, 2022

Should this be in draft mode, or is it ready for review?

aknuds1
aknuds1 reviewed Oct 18, 2022
View reviewed changes
pkg/usagestats/reporter.go Outdated Show resolved Hide resolved
aknuds1
aknuds1 reviewed Oct 18, 2022
View reviewed changes
pkg/usagestats/reporter.go Outdated Show resolved Hide resolved
@aknuds1 aknuds1 self-requested a review October 18, 2022 06:59
@aknuds1 aknuds1 added the enhancement New feature or request label Oct 18, 2022
pracucci
pracucci reviewed Oct 18, 2022
View reviewed changes
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Good job! I left a couple of minor comments. Next step is to add it to jsonnet and helm, but I'm happy if you follow up with separate PRs.

pkg/usagestats/reporter.go Outdated Show resolved Hide resolved
pkg/usagestats/reporter.go Outdated Show resolved Hide resolved
hi-rustin and others added 7 commits October 18, 2022 20:34
@hi-rustin @aknuds1
Update pkg/usagestats/reporter.go
cbd6efc 
Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>
@hi-rustin @aknuds1
Update pkg/usagestats/reporter.go
05b6ef8 
Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>
@hi-rustin @pracucci
Update pkg/usagestats/reporter.go
62927b9 
Co-authored-by: Marco Pracucci <marco@pracucci.com>
@hi-rustin
Address comments
a327575
@hi-rustin
Refine import
f0de835
@hi-rustin
Better code
b01795e
@hi-rustin
Update CHANGELOG.md
19ce930
@hi-rustin hi-rustin marked this pull request as ready for review October 18, 2022 12:58
@hi-rustin hi-rustin requested review from osg-grafana and a team as code owners October 18, 2022 12:58
@hi-rustin
Copy link
Contributor Author

hi-rustin commented Oct 18, 2022 •
edited
Loading

Next step is to add it to jsonnet and helm, but I'm happy if you follow up with separate PRs.

Thanks for your review. I also prefer to update it in the next PR because I need more time to update both the helm and the jsonnet templates.

@hi-rustin hi-rustin requested review from pracucci and removed request for aknuds1 and pstibrany October 18, 2022 13:03
@krajorama
Copy link
Contributor

Next step is to add it to jsonnet and helm, but I'm happy if you follow up with separate PRs.

Thanks for your review. I also prefer to update it in the next PR because I need more time to update both the helm and the jsonnet templates.

I think for helm it would make sense to add it as a CLI flag on all components and not part of the YAML config template. Since there would be no need to change it, doesn't affect how metrics are handled and makes it hard to overwrite by accident.

pracucci
pracucci approved these changes Oct 19, 2022
View reviewed changes
Copy link
Collaborator

@pracucci pracucci 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!

@pracucci pracucci enabled auto-merge (squash) October 19, 2022 06:55
@pracucci
Copy link
Collaborator

@hi-rustin Are you going to take care of adding the CLI flag to jsonnet and Helm, or should we delegate to someone else?

@pracucci
Copy link
Collaborator

Oh, I almost forgot. Please add an entry to docs/sources/operators-guide/configure/about-anonymous-usage-statistics-reporting.md to mention we're tracking the installation mode as well. In the section "Which information is collected", I would add a new entry under "Information about the Mimir cluster and version", something like "- The installation mode used to deploy Mimir, such as helm.".

@hi-rustin
Copy link
Contributor Author

Are you going to take care of adding the CLI flag to jsonnet and Helm, or should we delegate to someone else?

I'll take care it.

pracucci
pracucci approved these changes Oct 20, 2022
View reviewed changes
Copy link
Collaborator

@pracucci pracucci 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!

@pracucci pracucci enabled auto-merge (squash) October 20, 2022 06:35
@pracucci pracucci merged commit 3488332 into grafana:main Oct 20, 2022
@hi-rustin hi-rustin deleted the rustin-patch-track branch October 20, 2022 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aknuds1 aknuds1 aknuds1 left review comments

@pracucci pracucci pracucci approved these changes

@osg-grafana osg-grafana Awaiting requested review from osg-grafana

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@hi-rustin @aknuds1 @krajorama @pracucci