-
Notifications
You must be signed in to change notification settings - Fork 543
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
Periodically send anonymous usage stats report #2662
Conversation
const ( | ||
defaultReportCheckInterval = time.Minute | ||
defaultReportSendInterval = 4 * time.Hour | ||
defaultStatsServerURL = "https://stats.grafana.org/mimir-usage-report" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to reviewers: the server side API endpoint doesn't exist yet. I'm building the support in Mimir following the Loki specs.
if existing != nil { | ||
if s, ok := existing.(*expvar.String); ok { | ||
return s | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a NewString
function but it returns an existing string. I think this can hide undesired behaviors. Why would we expect this to overwrite a string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was copied from Loki, and I just kept logic as is. The way this stat function (and functions that will be introduced in the next PR) are intended to use, is just as a "get or create". I can change the name function to make it more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rename & unexport then.
|
||
nextReportAt = nextReport(r.reportSendInterval, seed.CreatedAt, time.Now()) | ||
case <-ctx.Done(): | ||
if err := ctx.Err(); !errors.Is(err, context.Canceled) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Offtopic: I feel like this should be done in the BasicService
, not in all the service implementers.
} | ||
|
||
func TestReporter_SendReportShouldSkipToNextReportOnLongFailure(t *testing.T) { | ||
t.Parallel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK this does nothing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends if there are tests with t.Parallel()
. For example the two following tests can be executed in parallel:
func TestOne(t *testing.T) {
t.Parallel()
// ...
}
func TestTwo(t *testing.T) {
t.Parallel()
// ...
}
I generally prefer to flag a slow test which is safe to run parallelly with t.Parallel()
without worrying if there are other parallel tests. If in the future we'll add them, they will run in parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM :)
Just some nitpicks on the tests.
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
75569e7
to
046804e
Compare
What this PR does
In this PR I'm adding the logic to periodically send anonymous usage stats report. The report doesn't contain custom metrics yet (will be done in in the next PR). The default stats we send (and the frequency) are the exact same of the ones sent by Loki, to keep consistency.
Which issue(s) this PR fixes or relates to
Part of #1815
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]