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

compact, receive, store: Init all labeled counter and histogram metrics #2893

Merged
merged 2 commits into from
Jul 23, 2020
Merged

compact, receive, store: Init all labeled counter and histogram metrics #2893

merged 2 commits into from
Jul 23, 2020

Conversation

yeya24
Copy link
Contributor

@yeya24 yeya24 commented Jul 15, 2020

Signed-off-by: Ben Ye yb532204897@gmail.com

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Fixes #2889

Verification

@yeya24
Copy link
Contributor Author

yeya24 commented Jul 15, 2020

I am not sure about this one https://github.com/thanos-io/thanos/blob/master/pkg/store/bucket.go#L198-L201. Why this is a counter metric, not a histogram or summary metric?

@kakkoyun kakkoyun changed the title Init all labeled counter and histogram metrics compact, receive, store: Init all labeled counter and histogram metrics Jul 20, 2020
kakkoyun
kakkoyun approved these changes Jul 20, 2020
View reviewed changes
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

💯 lgtm. Let's get one more opinion and we can merge it.

belm0
belm0 approved these changes Jul 20, 2020
View reviewed changes
Copy link
Contributor

@belm0 belm0 left a comment

Choose a reason for hiding this comment

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

LGTM from the humble issue reporter

@yeya24
Copy link
Contributor Author

yeya24 commented Jul 21, 2020

@pstibrany Hello, for this metric https://github.com/thanos-io/thanos/blob/master/pkg/store/bucket.go#L198-L201, is it intended to be a counter or a histogram? It doesn't have _total suffix

@pstibrany
Copy link
Contributor

@pstibrany Hello, for this metric https://github.com/thanos-io/thanos/blob/master/pkg/store/bucket.go#L198-L201, is it intended to be a counter or a histogram? It doesn't have _total suffix

Hi, it’s supposed to be counter and missing _total suffix is oversight :-(

@yeya24
Copy link
Contributor Author

yeya24 commented Jul 22, 2020

@pstibrany Hello, for this metric https://github.com/thanos-io/thanos/blob/master/pkg/store/bucket.go#L198-L201, is it intended to be a counter or a histogram? It doesn't have _total suffix

Hi, it’s supposed to be counter and missing _total suffix is oversight :-(

I can fix the name in another commit this pr if you don't mind.

@pstibrany
Copy link
Contributor

I can fix the name in another commit this pr if you don't mind.

That would be great, thanks a lot!

@yeya24
init all labeled counter and histogram metrics
6c27a82 
Signed-off-by: Ben Ye <yb532204897@gmail.com>
@yeya24
Copy link
Contributor Author

yeya24 commented Jul 22, 2020 •
edited
Loading

I can fix the name in another commit this pr if you don't mind.

That would be great, thanks a lot!

Done. Note that update is also needed on the Cortex side. https://github.com/cortexproject/cortex/blob/aa0d48adf6309a406e34b2d9e467787328312efb/pkg/storegateway/bucket_store_metrics.go#L111 😄

@pracucci
Copy link
Contributor

I can fix the name in another commit this pr if you don't mind.

That would be great, thanks a lot!

Done. Note that update is also needed on the Cortex side. https://github.com/cortexproject/cortex/blob/aa0d48adf6309a406e34b2d9e467787328312efb/pkg/storegateway/bucket_store_metrics.go#L111 😄

Definitely! We'll address it the next Thanos update in Cortex, once this PR is merged.

pracucci
pracucci approved these changes Jul 22, 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.

LGTM (modulo a fix in the downsampleFailures metric init)

cmd/thanos/compact.go Outdated
for _, meta := range sy.Metas() {
groupKey := compact.DefaultGroupKey(meta.Thanos)
downsampleMetrics.downsamples.WithLabelValues(groupKey)
downsampleMetrics.downsamples.WithLabelValues(groupKey)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
downsampleMetrics.downsamples.WithLabelValues(groupKey)
downsampleMetrics.downsampleFailures.WithLabelValues(groupKey)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks for catching this.

Btw just found another bug here https://github.com/thanos-io/thanos/blob/master/cmd/thanos/downsample.go#L234.

@yeya24
add _total suffix to thanos_bucket_store_cached_postings_compression_…
9714615 
…time_seconds

Signed-off-by: Ben Ye <yb532204897@gmail.com>
bwplotka
bwplotka approved these changes Jul 23, 2020
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

This is epic, thanks 💪

Some linter for that at some point would be nice ;p

@bwplotka bwplotka merged commit 8b96650 into thanos-io:master Jul 23, 2020
@yeya24 yeya24 deleted the init-countervec branch July 23, 2020 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwplotka bwplotka bwplotka approved these changes

@belm0 belm0 belm0 approved these changes

@pracucci pracucci pracucci approved these changes

@kakkoyun kakkoyun kakkoyun approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Init all labeled counter metrics to 0
6 participants
@yeya24 @pstibrany @pracucci @kakkoyun @belm0 @bwplotka