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

Runtime override of tenant-specific active series custom trackers #1188

Merged
merged 98 commits into from
Apr 5, 2022

Conversation

gubjanos
Copy link
Contributor

@gubjanos gubjanos commented Feb 15, 2022

What this PR does

This PR moves the active series custom trackers to a runtime configuration, and also introduces tenant-specific overwrites for these matchers.
I am uploading this to start an early discussion of the design,

Key points I would already like to discuss:

  • Moving the custom trackers to runtime configuration is a breaking change as it removes the flag and changes the default behavior
  • Config change management could be better handled on manager side, with some content hashing, but that would introduce dskit changes...

Which issue(s) this PR fixes

https://github.com/grafana/mimir-squad/issues/526

Fixes #

Checklist

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

@gubjanos gubjanos marked this pull request as draft February 15, 2022 10:23
@colega
Copy link
Contributor

colega commented Feb 15, 2022

I like the direction where this is going, however before going into a deeper review about nitpicks etc. I'd like to discuss the fact that per-tenant matchers overwrite the default ones.

IMO, the per-tenant matchers should be merged on top of the default ones. We could leave the door open to un-setting a matcher if needed (weird need, but who knows).

Consider that we have only one default matcher:

  • integration/caddy:...

And we set per-tenant matchers for a tenant:

  • integrations/caddy:... (we need to carry this default one)
  • team_a_metrics:...

After that, a system operator adds a new default one integration/k8s, but now they need to update all the per-tenant ones too, otherwise they'll get outdated.

@gubjanos
Copy link
Contributor Author

Actually, I have started the implementation in the way that you have just described.
I have decided to implement without merging because:

  • Based on our current configurations in jsonnet, it is easy to include the default set of matchers in tenant based configurations and it is not that hard to maintain. So the main idea was to keep it as simple as possible on ingester side, and offload the complexity to configuration
  • If we implement the default matchers + tenant-specific ones, we need to solve a complex config diffing problem (we need to check if the defaults change + the user specific ones) which result in more complex logic in the ingester
  • It is also not clear how to handle when the defaults and the user specific matchers have overlap

But all in all, I can easily be convinced to do the other way. :)

@colega
Copy link
Contributor

colega commented Feb 15, 2022

Oh I see, so you still want to merge but you want to do that in our jsonnet.

pkg/ingester/ingester.go Outdated Show resolved Hide resolved
pkg/ingester/ingester.go Outdated Show resolved Hide resolved
pkg/ingester/ingester.go Outdated Show resolved Hide resolved
pkg/ingester/ingester.go Outdated Show resolved Hide resolved
pkg/ingester/ingester.go Outdated Show resolved Hide resolved
pkg/ingester/ingester.go Outdated Show resolved Hide resolved
@gubjanos gubjanos requested a review from colega February 15, 2022 14:13
@gubjanos
Copy link
Contributor Author

One detail that's disturbing me a bit is terminology: technically "matcher" is a label="value" part (more correctly called "label matcher"), while {label="value", another=~"aaa.*"} is a selector. It would be a nice to fix the terminology, but it may be too late for that.

I agree with that. We have agreed to work on that in separate PR to limit the diff size of this one.

@gubjanos gubjanos self-assigned this Mar 31, 2022
@CLAassistant
Copy link

CLAassistant commented Mar 31, 2022

CLA assistant check
All committers have signed the CLA.

@gubjanos gubjanos changed the title Runtime matchers Runtime override of active series custom trackers Mar 31, 2022
@gubjanos gubjanos changed the title Runtime override of active series custom trackers Runtime override of tenant-specific active series custom trackers Mar 31, 2022
@pstibrany
Copy link
Member

Please rebase this PR on top of main to make all required test checks pass.

pkg/mimir/modules.go Outdated Show resolved Hide resolved
@colega
Copy link
Contributor

colega commented Mar 31, 2022

This looks good to me! Left a minor nitpick. Not leaving a green tick because as I mentioned I shouldn't be approving this since I collaborated on it.

Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Thank you. This is massive PR, but you did an amazing job navigating unfamiliar Mimir code and our comments and suggestions. Great job!

(I left few remaining small comments, but nothing blocking.)

pkg/ingester/activeseries/active_series.go Outdated Show resolved Hide resolved
cmd/mimir/help.txt.tmpl Outdated Show resolved Hide resolved
pkg/mimir/mimir_test.go Outdated Show resolved Hide resolved
pkg/mimir/mimir_test.go Outdated Show resolved Hide resolved
@pstibrany
Copy link
Member

Thank you. This is massive PR, but you did an amazing job navigating unfamiliar Mimir code and our comments and suggestions. Great job!

I forgot: @colega also did an amazing job on this PR. It would not be this far without your help!

One detail: there is comment about removing deprecated option in Mimir 2.2. I think it will need to be in 2.3, because this feature will be released in Mimir 2.1.

@pstibrany
Copy link
Member

Thank you for last touches on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants