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

Pass telemetry directly to the Controller #1248

Merged
merged 8 commits into from
Nov 9, 2024

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Nov 6, 2024

Do not enqueue telemetry to a channel just to dequeue it and send it to the Controller.

This requires that the Controller.Trace method is concurrent safe. The Controller is updated and tests are added to ensure this is the case.

Benchmark

package chbench

import (
	"sync/atomic"
	"testing"
)

func Benchmark(b *testing.B) {
	b.Run("Channel", func(b *testing.B) {
		var out atomic.Int64
		process := func(i int) { out.Store(int64(i)) }

		ch := make(chan int)
		go func() {
			for i := range ch {
				process(i)
			}
		}()

		b.ResetTimer()
		for n := 0; n < b.N; n++ {
			ch <- n
			for int(out.Load()) != n {
			}
		}
	})
	b.Run("Direct", func(b *testing.B) {
		var out atomic.Int64
		process := func(i int) { out.Store(int64(i)) }

		b.ResetTimer()
		for n := 0; n < b.N; n++ {
			process(n)
			for int(out.Load()) != n {
			}
		}
	})
}
$ go test . -count=10 -bench=. > out.txt && benchstat out.txt
goos: linux
goarch: amd64
pkg: testing/chbench
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
           │   out.txt   │
           │   sec/op    │
/Channel-8   31.39µ ± 4%
/Direct-8    8.325n ± 1%
geomean      511.2n

Do not enqueue telemetry to a channel just to dequeue it and send it to
the Controller.
Syntactic sugar for:

    var kvs []attribute.KeyValue
    kvs = appendAttrs(kvs, ptraceAttributeMap)
Use TracerProvider.Tracer directly instead of caching Tracers in the
Controller. The API ensures that TracerProvider.Tracer is concurrent
safe and the default SDK already handles the caching of Tracers (no need
to duplicate that logic here).
@MrAlias MrAlias marked this pull request as ready for review November 6, 2024 21:37
@MrAlias MrAlias requested a review from a team as a code owner November 6, 2024 21:37
@MrAlias MrAlias mentioned this pull request Nov 6, 2024
Copy link
Contributor

@damemi damemi left a comment

Choose a reason for hiding this comment

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

lgtm, just curious @MrAlias could you explain more what you did to make the controller concurrent-safe?

@MrAlias
Copy link
Contributor Author

MrAlias commented Nov 7, 2024

lgtm, just curious @MrAlias could you explain more what you did to make the controller concurrent-safe?

The controller's Trace method was calling getTracer which accessed a single tracer map. That map was not guarded by any semaphore and had data-races when accessed concurrently. This removes that code and instead uses the concurrent safe TracerProvider.Tracer method directly.

The default implementation of the SDK already provides a concurrent-safe cached lookup of a Tracer: https://github.com/open-telemetry/opentelemetry-go/blob/030ffdf4e46a496d0aa4693d488f8b5e534fd9bc/sdk/trace/provider.go#L125-L175. This switches to just using that instead of recreating.

@MrAlias MrAlias added this to the v0.18.0-alpha milestone Nov 7, 2024
@MrAlias MrAlias merged commit 535c9d8 into open-telemetry:main Nov 9, 2024
27 checks passed
@MrAlias MrAlias deleted the rm-telemetryCh branch November 9, 2024 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants