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

WIP sanitize structured metadata during ingestion in the distributor #15141

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cstyan
Copy link
Contributor

@cstyan cstyan commented Nov 27, 2024

Still working here, want to see if I can get the benchmark results to be any better.

The benchmark itself needs some love as well, if you use it with -count you can get a segmentation fault/nil pointer dereference panic in the mock ingester code.

Just the addition of the two checks slows down distributor.Push significantly, and also it looks to me like when the log line doesn't contain a log field for otlp somehow we might be adding the unknown value multiple times? [{detected_level unknown} {detected_level unknown} {detected_level unknown} {detected_level unknown}]

Very basic benchmark results so far:

  • main is off main f65ab130725dc25c9d546fa4d5fb1e4a6d26009e
  • main with sm is with the modification to makeWriteRequestWithLabels to add structured metadata
  • pr-only-value is with the addition of the check in distributor.Push to check and sanitize the label value
  • pr-both is with the addition of both the SM name and value check + sanitization

Note that this is worst case scenario, since every entry in the write request has structured metadata that needs to be checked

goos: linux
goarch: amd64
pkg: github.com/grafana/loki/v3/pkg/distributor
cpu: AMD Ryzen 9 5950X 16-Core Processor
                           │    main     │             main-with-sm             │             pr-only-value             │                 pr-both                 │
                           │   sec/op    │    sec/op     vs base                │    sec/op     vs base                 │    sec/op      vs base                  │
_Push-32                     53.22m ± 1%   53.74m ± ∞ ¹       ~ (p=0.250 n=7+1)   62.81m ± ∞ ¹        ~ (p=0.250 n=7+1)   107.62m ± ∞ ¹         ~ (p=0.250 n=7+1)
_PushWithLineTruncation-32                 57.32m ± ∞ ¹                           64.91m ± ∞ ¹                            110.90m ± ∞ ¹
geomean                      53.22m        55.50m        +0.98%                   63.85m        +18.03%                    109.2m        +102.23%
¹ need >= 6 samples for confidence interval at level 0.95

                           │     main     │             main-with-sm              │             pr-only-value             │                pr-both                 │
                           │     B/op     │     B/op       vs base                │     B/op       vs base                │     B/op       vs base                 │
_Push-32                     6.653Mi ± 3%   7.217Mi ± ∞ ¹       ~ (p=0.250 n=7+1)   7.219Mi ± ∞ ¹       ~ (p=0.250 n=7+1)   8.096Mi ± ∞ ¹        ~ (p=0.250 n=7+1)
_PushWithLineTruncation-32                  7.232Mi ± ∞ ¹                           7.501Mi ± ∞ ¹                           8.116Mi ± ∞ ¹
geomean                      6.653Mi        7.225Mi        +8.47%                   7.359Mi        +8.51%                   8.106Mi        +21.69%
¹ need >= 6 samples for confidence interval at level 0.95

                           │    main     │             main-with-sm             │            pr-only-value             │               pr-both                │
                           │  allocs/op  │  allocs/op    vs base                │  allocs/op    vs base                │  allocs/op    vs base                │
_Push-32                     20.09k ± 5%   18.09k ± ∞ ¹       ~ (p=0.250 n=7+1)   18.13k ± ∞ ¹       ~ (p=0.250 n=7+1)   20.44k ± ∞ ¹       ~ (p=0.500 n=7+1)
_PushWithLineTruncation-32                 18.31k ± ∞ ¹                           19.00k ± ∞ ¹                           20.74k ± ∞ ¹
geomean                      20.09k        18.20k        -9.94%                   18.56k        -9.75%                   20.59k        +1.74%
¹ need >= 6 samples for confidence interval at level 0.95

@@ -519,6 +530,12 @@ func (d *Distributor) Push(ctx context.Context, req *logproto.PushRequest) (*log
structuredMetadata := logproto.FromLabelAdaptersToLabels(entry.StructuredMetadata)
if shouldDiscoverLevels {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we do this only for should DiscoverLevels ?

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

Successfully merging this pull request may close these issues.

2 participants