Use auto-instrumentation SDK in global tracing #5920

MrAlias · 2024-10-24T23:52:13Z

When the auto-instrumentation attaches to a process using the global TracerProvider, and there has not been a delegate set, create a span from the go.opentelemetry.io/auto/sdk package so the OTel Go auto-instrumentation can instrument the application by default.

Resolve #5702

Benchmarks

goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/internal/global
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
                    │   old.txt    │            new.txt             │
                    │    sec/op    │    sec/op     vs base          │
StartEndSpanNoSDK-8   244.2n ± 21%   250.2n ± 23%  ~ (p=0.739 n=10)

codecov · 2024-10-25T00:09:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.0%. Comparing base (078b2dd) to head (c39b4c1).

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #5920     +/-   ##
=======================================
- Coverage   84.6%   82.0%   -2.6%     
=======================================
  Files        272     273      +1     
  Lines      22839   23571    +732     
=======================================
+ Hits       19325   19344     +19     
- Misses      3170    3883    +713     
  Partials     344     344

see 4 files with indirect coverage changes

When the auto-instrumentation attaches to a process using the global TracerProvider, and there has not been a delegate set, create a span from the go.opentelemetry.io/auto/sdk package so the OTel Go auto-instrumentation can instrument the application by default.

internal/global/trace.go

pellared · 2024-10-28T11:34:04Z

go.mod


 require (
 	github.com/go-logr/logr v1.4.2
 	github.com/go-logr/stdr v1.2.2
 	github.com/google/go-cmp v0.6.0
 	github.com/stretchr/testify v1.9.0
+	go.opentelemetry.io/auto/sdk v0.1.0-alpha


Wouldn't coupling most modules of OTel Go to an unstable dependency occur too problematic for the users? I think the probability of a conflict is very low, but I would feel safer if we describe the possible consequences even in the PR description.

I think that as long as only the API modules are using go.opentelemetry.io/auto/sdk directly then it won't be an issue.

However, the user may have trouble if they also want to directly use go.opentelemetry.io/auto/sdk. Is it possible that a user may want to use API packages from go.opentelemetry.io/otel and instrumentation libraries, use instrumentation libraries for capturing telemetry and configure go.opentelemetry.io/auto/sdk on their own (the users would not use OTel SDK at all)? Given the package description of https://pkg.go.dev/go.opentelemetry.io/auto/sdk, I think that such scenario is possible then users of this module would get build issues each time there is a breaking change in https://pkg.go.dev/go.opentelemetry.io/auto/sdk and before they switch the newer version of OTel API that uses the new changes of auto/sdk.

This looks like a sort of a circular dependency.

Maybe auto could be sub-packages of API modules? It looks bad, but this is what this PR kind of does implicitly.

This looks like a sort of a circular dependency.

sdk never imports otel or auto. How do you see the circular dependency?

Maybe auto could be sub-packages of API modules? It looks bad, but this is what this PR kind of does implicitly.

auto is a full auto-instrumentation package. It is run as a separate process to manage eBPF instrumentation. I'm not following why we would want that as a sub-package of the API modules?

This looks like a sort of a circular dependency.

sdk never imports otel or auto. How do you see the circular dependency?

I mean auto/sdk and otel Go modules

https://pkg.go.dev/go.opentelemetry.io/auto/sdk?tab=imports

Circular dependency of Go modules (not packages)

Circular dependency of Go modules (not packages)

Gotcha. Yeah, that's not a problem for Go. It's something we tested when developing that module and didn't have any issues.

I think it may be an acceptable design trade-off. More https://github.com/open-telemetry/opentelemetry-go/pull/5920/files#r1819394158

pellared · 2024-10-28T16:39:11Z

internal/global/trace.go

+	// should not be read from the global.
+
+	if *autoSpan {
+		tracer := sdk.GetTracerProvider().Tracer(t.name, t.opts...)


As long as only sdk.GetTracerProvider() is here and its signalture is not changed then the circular dependency of Go modules should not be a problem. I think we should make some automation e.g. using golangci-lint to make sure we will limit usage of auto/sdk to bare minimum. Also in auto/sdk add something to minimize the possibility of changing this function signature.

Yeah, I guess the ask is if we can make auto/sdk stable.

I'm not opposed to bringing that request back to the auto SIG. But it may mean there is a greater chance of a v2 for this package if we find an issue implementing this here.

Alternativly it may be good enough to request stability of only this one function without making a V1.

Alternativly it may be good enough to request stability of only this one function without making a V1.

I mean, that I can almost guarantee. It's just that it would only be codified in a commented agreement between developers instead of a version number.

Another thing I'm thinking through is the embedding of the default type for the Tracer and Span from that package. If we add a method to those interfaces here, we would need to update the sdk pkg with some implementation and include it in the release that would add the methods here. Otherwise, there would be compilation error of the otel package.

That is something we could do. But, I'm wondering if we should be embedding a noop there`.

That is something we could do. But, I'm wondering if we should be embedding a noop there`.

You mean change https://github.com/open-telemetry/opentelemetry-go-instrumentation/blob/f9241162a20bd4beb20d4347db3d8a77280920b1/sdk/trace.go#L34

to type tracerProvider struct{ noop.TracerProvider }?

If so then, I think it would be good and should mitigate a chicken-egg problem when introducing a new method to Trace API.

Yeah, that's what I'm thinking: 👍

Shouldn't be needed for this PR, but would be needed before we release this.

Updated depguard. There is no way to ignore use outside of the import statement. I've just ignored these files.

@pellared should we just have the auto/sdk{,/telemetry} packages live here?

We originally published them under auto to allow development in the SIG they are designed for, but I wonder if the better user-story is one where they are hosted under otel/internal/global/...?

It would add code for auto-instrumentation here and add development burden, but it would also ensure there is never an otel release that could be broke by an independent upgrade of auto/sdk.

@pellared should we just have the auto/sdk{,/telemetry} packages live here?

We originally published them under auto to allow development in the SIG they are designed for, but I wonder if the better user-story is one where they are hosted under otel/internal/global/...?

It would add code for auto-instrumentation here and add development burden, but it would also ensure there is never an otel release that could be broke by an independent upgrade of auto/sdk.

This would require open-telemetry/opentelemetry-go-instrumentation#1207 or something similar so we would no longer depend on auto/sdk/telemetry in the auto module.

We originally published them under auto to allow development in the SIG they are designed for, but I wonder if the better user-story is one where they are hosted under otel/internal/global/...?

I have a "feeling" that this would be a better design as this repository would be less (not?) dependent on go.opentelemetry.io/auto. Maybe it would be good to explore?

PS. This is kind of what I meant by

Maybe auto could be sub-packages of API modules?

Sorry that my comments are not very precise, but I also also having unprecise thoughts 😅

Do not allow use of auto/sdk outside of global pkg.

pellared

Discussed offline with @MrAlias.

We think that before merging this it would be good to:

rename sdk.GetTracerProvider to sdk.TracerProvider; reason: https://go.dev/doc/effective_go#Getters
make a v1 release of go.opentelemetry.io/auto/sdk

.golangci.yml

internal/global/trace.go

MrAlias force-pushed the global-auto-sdk branch from a684c84 to 38fc53a Compare October 25, 2024 00:05

MrAlias force-pushed the global-auto-sdk branch from 714c8a2 to 38ddafd Compare October 25, 2024 14:28

MrAlias added 4 commits October 25, 2024 07:52

Run go mod tidy

f8191b8

Add changelog entry

5eb4966

Add TestNewSpanType

85a116d

MrAlias force-pushed the global-auto-sdk branch from 3e39369 to 85a116d Compare October 25, 2024 14:52

MrAlias marked this pull request as ready for review October 25, 2024 14:59

MrAlias requested review from XSAM, dashpole, pellared and dmathieu as code owners October 25, 2024 14:59

dashpole reviewed Oct 25, 2024

View reviewed changes

internal/global/trace.go Show resolved Hide resolved

MrAlias force-pushed the global-auto-sdk branch from 155c165 to 85a116d Compare October 25, 2024 15:29

dashpole approved these changes Oct 25, 2024

View reviewed changes

pellared reviewed Oct 28, 2024

View reviewed changes

MrAlias and others added 2 commits October 28, 2024 10:39

Merge branch 'main' into global-auto-sdk

fb171ae

Update depguard rules

c39b4c1

Do not allow use of auto/sdk outside of global pkg.

dmathieu approved these changes Oct 29, 2024

View reviewed changes

pellared reviewed Oct 29, 2024

View reviewed changes

XSAM approved these changes Oct 29, 2024

View reviewed changes

.golangci.yml Show resolved Hide resolved

XSAM reviewed Oct 29, 2024

View reviewed changes

internal/global/trace.go Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use auto-instrumentation SDK in global tracing #5920

Use auto-instrumentation SDK in global tracing #5920

MrAlias commented Oct 24, 2024 •

edited

Loading

codecov bot commented Oct 25, 2024 •

edited

Loading

pellared Oct 28, 2024 •

edited

Loading

MrAlias Oct 28, 2024 •

edited

Loading

MrAlias Oct 28, 2024

pellared Oct 28, 2024 •

edited

Loading

MrAlias Oct 28, 2024

pellared Oct 28, 2024 •

edited

Loading

pellared Oct 28, 2024 •

edited

Loading

MrAlias Oct 28, 2024

pellared Oct 28, 2024

MrAlias Oct 28, 2024

MrAlias Oct 28, 2024

MrAlias Oct 28, 2024

MrAlias Oct 28, 2024

MrAlias Oct 28, 2024

MrAlias Oct 28, 2024

pellared Oct 29, 2024 •

edited

Loading

pellared left a comment

Use auto-instrumentation SDK in global tracing #5920

Are you sure you want to change the base?

Use auto-instrumentation SDK in global tracing #5920

Conversation

MrAlias commented Oct 24, 2024 • edited Loading

Benchmarks

codecov bot commented Oct 25, 2024 • edited Loading

Codecov Report

pellared Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

MrAlias Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pellared Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pellared Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

pellared Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pellared Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

pellared left a comment

Choose a reason for hiding this comment

MrAlias commented Oct 24, 2024 •

edited

Loading

codecov bot commented Oct 25, 2024 •

edited

Loading

pellared Oct 28, 2024 •

edited

Loading

MrAlias Oct 28, 2024 •

edited

Loading

pellared Oct 28, 2024 •

edited

Loading

pellared Oct 28, 2024 •

edited

Loading

pellared Oct 28, 2024 •

edited

Loading

pellared Oct 29, 2024 •

edited

Loading