Skip to content

ingester v2: supports statistics in blocks storage #2042

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
Merged
merged 3 commits into from
Jan 29, 2020
Merged

ingester v2: supports statistics in blocks storage #2042

merged 3 commits into from
Jan 29, 2020

Conversation

kamijin-fanta
Copy link
Contributor

@kamijin-fanta kamijin-fanta commented Jan 28, 2020 •
edited
Loading

What this PR does:

Make /all_user_stats page available when using BlocksStorage.

Which issue(s) this PR fixes:
Fixes #1825

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]
@kamijin-fanta
Copy link
Contributor Author

If this PR is acceptable, I will write test for pkg/ingester/rate.go and add changelog.
pracucci
pracucci reviewed Jan 28, 2020
View reviewed changes
Copy link
Contributor

@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.

Excellent job! We definitely want it!

The changes looks good so far. Few things:

  1. As you mentioned, would be great if you could add a unit test on v2AllUserStats()
  2. Could you also implement v2UserStats(), please?
  3. Could you add an entry to the CHANGELOG.md (see the format of other entries and remember to add the PR number too)
pkg/ingester/ingester_v2.go Outdated
select {
case <-rateUpdateTicker.C:
i.userStatesMtx.RLock()
for _, state := range i.TSDBState.dbs {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rename state into db for consistency with the rest of the code base.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. i will rename that.
pkg/ingester/rate.go Outdated
@@ -51,3 +51,8 @@ func (r *ewmaRate) tick() {
func (r *ewmaRate) inc() {
atomic.AddInt64(&r.newEvents, 1)
}

// add counts some event.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure it's worth this comment. The function looks pretty straightforward.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

can i remove this line?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think so.
@kamijin-fanta
Copy link
Contributor Author

thank you! make improvements according to you.
pracucci
pracucci approved these changes Jan 28, 2020
View reviewed changes
Copy link
Contributor

@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.

Very good job! I left few minor comments, but LGTM 👏
CHANGELOG.md Outdated
@@ -59,6 +59,7 @@ instructions below to upgrade your Postgres.
* [BUGFIX] TSDB: Fixed `cortex_ingester_memory_series` and `cortex_ingester_memory_users` metrics when using with the experimental TSDB blocks storage. #1982
* [BUGFIX] TSDB: Fixed `cortex_ingester_memory_series_created_total` and `cortex_ingester_memory_series_removed_total` metrics when using TSDB blocks storage. #1990
* [BUGFIX] Table Manager: Fixed calculation of expected tables and creation of tables from next active schema considering grace period. #1976
* [FEATURE] TSDB: `/all_user_stats` page now available. #2042
Copy link
Contributor

Choose a reason for hiding this comment

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

A couple of things:

  • This looks more a [BUGFIX] than [FEATURE]
  • I would suggest [BUGFIX] Experimental TSDB: fixed `/all_user_stats` and `/api/prom/user_stats` endpoints when using the experimental TSDB blocks storage. #2042
pkg/ingester/ingester_v2.go Outdated
Comment on lines 445 to 452
apiRate := db.ingestedAPISamples.rate()
ruleRate := db.ingestedRuleSamples.rate()
return &client.UserStatsResponse{
IngestionRate: apiRate + ruleRate,
ApiIngestionRate: apiRate,
RuleIngestionRate: ruleRate,
NumSeries: db.Head().NumSeries(),
}, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is duplicated with v2AllUserStats(). Would you mind adding a new function like createUserStats(db) which creates the user stat for 1 user, which we can directly return here and append() in the v2AllUserStats(), please?
pkg/ingester/ingester_v2_test.go Outdated
{"user-1", labels.Labels{{Name: labels.MetricName, Value: "test_1_2"}}, 2, 200000},
{"user-2", labels.Labels{{Name: labels.MetricName, Value: "test_2_1"}}, 2, 200000},
{"user-2", labels.Labels{{Name: labels.MetricName, Value: "test_2_2"}}, 2, 200000},
{"user-2", labels.Labels{{Name: labels.MetricName, Value: "test_2_3"}}, 2, 200000},
Copy link
Contributor

Choose a reason for hiding this comment

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

Despite correct, I would suggest to push a different number of series for the user-2 (ie. remove this line) to have an higher confidence seeing different assertions below in the test.
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 avoided float number. but your comment is right and will be fixed.
@kamijin-fanta
Copy link
Contributor Author

I'm not used to contributing, so thanks for the kind comments!
pracucci
pracucci approved these changes Jan 28, 2020
View reviewed changes
Copy link
Contributor

@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.

I'm not used to contributing, so thanks for the kind comments!

You did a great job! I would love seeing more contributions from you ❤️
@pracucci
Copy link
Contributor

@kamijin-fanta do you mind rebasing master, please? We've merge the release 0.6.0 and you would need to move your CHANGELOG.md entry to the top of the file (remember to keep entries grouped by type).
@kamijin-fanta
ingester v2: supports statistics in blocks storage
8f55ec2 
Signed-off-by: kamijin_fanta <kamijin@live.jp>
@kamijin-fanta
ingester v2: add UserStats tests
8631256 
Signed-off-by: kamijin_fanta <kamijin@live.jp>
@kamijin-fanta
ingester v2: fix AllUserStats tests
40d6399 
Signed-off-by: kamijin_fanta <kamijin@live.jp>
@kamijin-fanta
Copy link
Contributor Author

done. please check it!
jtlisi
jtlisi approved these changes Jan 28, 2020
View reviewed changes
Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

LGTM
@pracucci pracucci merged commit 47465f1 into cortexproject:master Jan 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants
@kamijin-fanta @pracucci @jtlisi