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

tailsamplingprocessor: Fix UT issue after mutator memory allocations PR #28597

Closed
wants to merge 5 commits into from

Conversation

jiekun
Copy link
Member

@jiekun jiekun commented Oct 25, 2023

Description: In #27889, we have a optimization thanks to @brancz . The optimization introduce a new field for tailSamplingSpanProcessor, which is not initialized during unit test, and cause panic.

--- FAIL: TestTraceIntegrity (0.00s)
panic: runtime error: index out of range [0] with length 0 [recovered]
	panic: runtime error: index out of range [0] with length 0

goroutine 15 [running]:
testing.tRunner.func1.2({0x112df00, 0xc00016a5a0})
	/opt/hostedtoolcache/go/1.20.10/x64/src/testing/testing.go:1526 +0x372
testing.tRunner.func1()
	/opt/hostedtoolcache/go/1.20.10/x64/src/testing/testing.go:1529 +0x650
panic({0x112df00, 0xc00016a5a0})
	/opt/hostedtoolcache/go/1.20.10/x64/src/runtime/panic.go:890 +0x263
github.com/open-telemetry/opentelemetry-collector-contrib/processor/tailsamplingprocessor.(*tailSamplingSpanProcessor).makeDecision(0xc000326180, {0x1, 0x2, 0x3, 0x4, 0x0, 0x0, 0x0, 0x0, 0x0, ...}, ...)
	/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/processor/tailsamplingprocessor/processor.go:305 +0x14b9
github.com/open-telemetry/opentelemetry-collector-contrib/processor/tailsamplingprocessor.(*tailSamplingSpanProcessor).samplingPolicyOnTick(0xc000326180)

root cause code block:

        // We use this field here, which is not initialized in unit test, so it's still a `nil`
	mutators := tsp.mutatorsBuf
	for i, p := range tsp.policies {
                ...
                        // panic here.
			mutators[0] = tagUpsertSampled

I simply add 1 line to related test cases to have them fixed and could be merge into upstream quicker.

	tsp := &tailSamplingSpanProcessor{
                ...

                // Here
		mutatorsBuf:     make([]tag.Mutator, 1),
	}

Link to tracking Issue: Check the GitHub Action log of #27889

Testing: It's a fix for the unit test so I think no extra test case needed.

Documentation: Doc and changelog already added by @brancz in original PR. No documentation needed in this PR.

brancz and others added 4 commits October 20, 2023 17:39
Since each `tailSamplingSpanProcessor`'s instance is not concurrently
called by the ticker worker (it's a 1-to-1 relationship) we can safely
reuse a slice for the tag mutators used in `makeDecision`. Additionally
the tag mutators themselves were causing a lot of allocations and since
they are static, we created constants for them preventing allocations on
each execution of `makeDecision`.

This improved the `makeDecision` benchmark by ~31%.

```
benchstat old.txt new.txt
name         old time/op  new time/op  delta
Sampling-10  51.8µs ± 1%  35.7µs ± 1%  -30.94%  (p=0.008 n=5+5)
```
…ield mutatorsBuf and panic in CICD. Added this field to all struct in ut.
@jiekun
Copy link
Member Author

jiekun commented Oct 25, 2023

Alright sorry for the lint issue. Let me run it locally and push again :/

@jiekun jiekun closed this Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
processor/tailsampling Tail sampling processor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants