-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
feat: Introduce policy stream mapping #15982
base: main
Are you sure you want to change the base?
Conversation
…na/loki into retention-as-label-when-discarding
87272ca
to
243f34e
Compare
…na/loki into add-stream-mapping-to-loki
💻 Deploy preview available: https://deploy-preview-loki-15982-zb444pucvq-vp.a.run.app/docs/loki/latest/ |
@@ -3927,6 +3927,9 @@ otlp_config: | |||
# CLI flag: -validation.enforced-labels | |||
[enforced_labels: <list of strings> | default = []] | |||
|
|||
# Map of policies to stream selectors with a priority. Experimental. |
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.
Is there a default for this setting? (even if the default is nothing is set?)
Also, this could use an example in the description, since users might not know what a mapping should look like.
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.
No, default means no mapping and no policies assigned.
But good call, I'll improve the docs of this struct.
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.
Pushed a docs example, wdyt?
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.
Nice! So glad you added the example, as that's pretty complicated.
@@ -540,3 +545,40 @@ func TestExpirationChecker_IntervalMayHaveExpiredChunks(t *testing.T) { | |||
}) | |||
} | |||
} | |||
|
|||
func Test_PolicyStreamMapping_PolicyFor(t *testing.T) { |
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 great! I think we should make it a bit more complete tho:
- Two mapping policies with the same priority: Get first one matchiong. We should log an error with insight logs and some metrics but we can do that in a followup PR (I added a task to the epic for this)
- Regexes
- Multiple mappings per policy
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.
Pushed a commit addressing those scenarios.
for _, policyStream := range policyStreams { | ||
for _, m := range policyStream.Matchers { | ||
if !m.Matches(lbs.Get(m.Name)) { | ||
continue Outer |
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 think this is not correct. IIUC here if the labels do not match the matcher for this policyStream, we continue to the next policy instead of to the next policyStream within this policy.
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.
good find. I fixed its logic, wdyt?
found bool | ||
) | ||
Outer: | ||
for policyName, policyStreams := range policyStreamMapping { |
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 loop is pretty similar to the RetentionPeriodFor
. Still I cannot help but to think this is a O(n*m) loop that we do for every single push request we get.
Probably not a big deal if customers configure just a few mappings similarly to stream retention.
As we discussed we may need some caching here, but I'm ok with benchmarking this to see if it's possible or even doing it in a followup PR.
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.
hey I pushed a commit with a benchmark that I ran today. The results are:
BenchmarkDistributor_PushWithPolicies/push_without_policies-24 87802 13223 ns/op 7468 B/op 67 allocs/op
BenchmarkDistributor_PushWithPolicies/push_with_1_policies-24 82648 13417 ns/op 7474 B/op 67 allocs/op
BenchmarkDistributor_PushWithPolicies/push_with_10_policies-24 82141 13328 ns/op 7478 B/op 67 allocs/op
BenchmarkDistributor_PushWithPolicies/push_with_100_policies-24 79417 14192 ns/op 7485 B/op 67 allocs/op
BenchmarkDistributor_PushWithPolicies/push_with_1_matchers-24 84045 13316 ns/op 7489 B/op 67 allocs/op
BenchmarkDistributor_PushWithPolicies/push_with_10_matchers-24 83119 13501 ns/op 7495 B/op 67 allocs/op
BenchmarkDistributor_PushWithPolicies/push_with_100_matchers-24 76173 14551 ns/op 7504 B/op 67 allocs/op
PASS
So caching isn't looking necessary.
What this PR does / why we need it:
Introduces the idea of policies to Loki, which are recognizable based on the given stream selectors.
This is an improved version of #15561 and built on top of #15875.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Checklist
CONTRIBUTING.md
guide (required)feat
PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.md
deprecated-config.yaml
anddeleted-config.yaml
files respectively in thetools/deprecated-config-checker
directory. Example PR