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

model: clone labels before sanitizing #8652

Merged
merged 1 commit into from
Jul 20, 2022

Conversation

axw
Copy link
Member

@axw axw commented Jul 20, 2022

Motivation/summary

If an agent sends global labels, then events without event-specific labels will share the same map of global labels. If those global labels need sanitisation, then the server may panic due to concurrently modifying/reading the map. To prevent this, clone the labels map if sanitisation is required.

Checklist

- [ ] Update CHANGELOG.asciidoc to be added at release time, to avoid updating live docs
- [ ] Update package changelog.yml (only if changes to apmpackage have been made)
- [ ] Documentation has been updated

How to test these changes

Send a continuous stream of events with global labels that require sanitisation (e.g. a label key with a dot in it), and no event-specific labels. The server should not panic.

Related issues

Closes #8651

@apmmachine
Copy link
Contributor

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-07-20T01:52:48.273+0000

  • Duration: 42 min 57 sec

Test stats 🧪

Test Results
Failed 0
Passed 5967
Skipped 22
Total 5989

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /hey-apm : Run the hey-apm benchmark.

  • /package : Generate and publish the docker images.

  • /test windows : Build & tests on Windows.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@apmmachine
Copy link
Contributor

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 97.917% (47/48) 👍
Files 91.705% (199/217) 👍
Classes 92.662% (442/477) 👍
Methods 89.101% (1120/1257) 👍
Lines 76.749% (13613/17737) 👍 0.056
Conditionals 100.0% (0/0) 💚

@apmmachine
Copy link
Contributor

📚 Go benchmark report

Diff with the 7.17 branch

name                                                                                                                                             old time/op    new time/op    delta
pkg:github.com/elastic/apm-server/agentcfg goos:linux goarch:amd64
pkg:github.com/elastic/apm-server/model/modelindexer goos:linux goarch:amd64
ModelIndexer/BestCompression-12                                                                                                                    12.5µs ±31%    22.6µs ±20%  +80.67%  (p=0.008 n=5+5)
pkg:github.com/elastic/apm-server/processor/stream goos:linux goarch:amd64
BackendProcessor/invalid-metadata.ndjson-12                                                                                                        2.47µs ± 1%    2.39µs ± 1%   -3.05%  (p=0.008 n=5+5)
BackendProcessor/minimal-service.ndjson-12                                                                                                         9.79µs ±15%   12.75µs ±27%  +30.22%  (p=0.032 n=5+5)
BackendProcessor/transactions.ndjson-12                                                                                                            91.4µs ±45%   155.2µs ±20%  +69.80%  (p=0.032 n=5+5)
RUMV3Processor/rum_events.ndjson-12                                                                                                                 132µs ±31%     207µs ±24%  +57.16%  (p=0.016 n=5+5)
pkg:github.com/elastic/apm-server/publish goos:linux goarch:amd64
pkg:github.com/elastic/apm-server/utility goos:linux goarch:amd64
ExtractIP/extractIP_remote:_nil,_Forwarded:_nil,_X-Real-IP:_nil,_X-Forwarded-For:_nil-12                                                            154ns ±11%     136ns ± 5%  -11.84%  (p=0.008 n=5+5)
pkg:github.com/elastic/apm-server/x-pack/apm-server/aggregation/spanmetrics goos:linux goarch:amd64
pkg:github.com/elastic/apm-server/x-pack/apm-server/aggregation/txmetrics goos:linux goarch:amd64
AggregateTransaction-12                                                                                                                            68.2ns ± 1%    69.1ns ± 1%   +1.38%  (p=0.008 n=5+5)
pkg:github.com/elastic/apm-server/x-pack/apm-server/sampling goos:linux goarch:amd64
pkg:github.com/elastic/apm-server/x-pack/apm-server/sampling/eventstorage goos:linux goarch:amd64
WriteTransaction/nop_codec-12                                                                                                                       157ns ± 3%     146ns ± 1%   -7.29%  (p=0.008 n=5+5)
IsTraceSampled/sampled-12                                                                                                                          80.1ns ± 2%    78.0ns ± 2%   -2.66%  (p=0.032 n=5+5)
IsTraceSampled/unsampled-12                                                                                                                        79.6ns ± 3%    75.4ns ± 3%   -5.22%  (p=0.016 n=5+5)

name                                                                                                                                             old alloc/op   new alloc/op   delta
pkg:github.com/elastic/apm-server/agentcfg goos:linux goarch:amd64
pkg:github.com/elastic/apm-server/model/modelindexer goos:linux goarch:amd64
ModelIndexer/BestCompression-12                                                                                                                    2.94kB ± 1%    3.12kB ± 4%   +6.09%  (p=0.008 n=5+5)
pkg:github.com/elastic/apm-server/processor/stream goos:linux goarch:amd64
BackendProcessor/metricsets.ndjson-12                                                                                                              43.1kB ± 1%    41.9kB ± 1%   -2.64%  (p=0.008 n=5+5)
BackendProcessor/transactions.ndjson-12                                                                                                            50.1kB ± 3%    48.0kB ± 2%   -4.10%  (p=0.008 n=5+5)
pkg:github.com/elastic/apm-server/publish goos:linux goarch:amd64
pkg:github.com/elastic/apm-server/utility goos:linux goarch:amd64
pkg:github.com/elastic/apm-server/x-pack/apm-server/aggregation/spanmetrics goos:linux goarch:amd64
pkg:github.com/elastic/apm-server/x-pack/apm-server/aggregation/txmetrics goos:linux goarch:amd64
pkg:github.com/elastic/apm-server/x-pack/apm-server/sampling goos:linux goarch:amd64
pkg:github.com/elastic/apm-server/x-pack/apm-server/sampling/eventstorage goos:linux goarch:amd64
ReadEvents/nop_codec/100_events-12                                                                                                                  225kB ± 0%     225kB ± 0%   +0.26%  (p=0.032 n=5+5)

name                                                                                                                                             old allocs/op  new allocs/op  delta
pkg:github.com/elastic/apm-server/agentcfg goos:linux goarch:amd64
pkg:github.com/elastic/apm-server/model/modelindexer goos:linux goarch:amd64
pkg:github.com/elastic/apm-server/processor/stream goos:linux goarch:amd64
pkg:github.com/elastic/apm-server/publish goos:linux goarch:amd64
pkg:github.com/elastic/apm-server/utility goos:linux goarch:amd64
pkg:github.com/elastic/apm-server/x-pack/apm-server/aggregation/spanmetrics goos:linux goarch:amd64
pkg:github.com/elastic/apm-server/x-pack/apm-server/aggregation/txmetrics goos:linux goarch:amd64
pkg:github.com/elastic/apm-server/x-pack/apm-server/sampling goos:linux goarch:amd64
pkg:github.com/elastic/apm-server/x-pack/apm-server/sampling/eventstorage goos:linux goarch:amd64

name                                                                                                                                             old speed      new speed      delta
pkg:github.com/elastic/apm-server/processor/stream goos:linux goarch:amd64
BackendProcessor/invalid-metadata.ndjson-12                                                                                                       181MB/s ± 1%   187MB/s ± 1%   +3.15%  (p=0.008 n=5+5)
BackendProcessor/minimal-service.ndjson-12                                                                                                       43.8MB/s ±16%  34.4MB/s ±24%  -21.30%  (p=0.032 n=5+5)
BackendProcessor/transactions.ndjson-12                                                                                                          66.4MB/s ±50%  36.9MB/s ±20%  -44.38%  (p=0.032 n=5+5)
RUMV3Processor/rum_events.ndjson-12                                                                                                              30.7MB/s ±26%  19.4MB/s ±29%  -36.97%  (p=0.016 n=5+5)

report generated with https://pkg.go.dev/golang.org/x/perf/cmd/benchstat

@axw axw requested a review from a team July 20, 2022 02:42
Copy link
Contributor

@lahsivjar lahsivjar left a comment

Choose a reason for hiding this comment

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

LGTM

@axw axw merged commit 5e74a90 into elastic:7.17 Jul 20, 2022
@axw axw deleted the global-labels-sanitization branch July 20, 2022 05:06
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.

3 participants