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

Add instrumentation for the "go.opentelemetry.io/otel/trace".nonRecordingSpan #974

Open
MrAlias opened this issue Jul 30, 2024 · 5 comments · Fixed by #1405
Open

Add instrumentation for the "go.opentelemetry.io/otel/trace".nonRecordingSpan #974

MrAlias opened this issue Jul 30, 2024 · 5 comments · Fixed by #1405
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Jul 30, 2024

A common instrumentation pattern for tracing is to return the the TracerProvider from the span held in a context:

import (
	"net/http"

	"go.opentelemetry.io/otel/trace"
)

func handler(w http.ResponseWriter, r *http.Request) {
	ctx := r.Context()
	tracer := trace.SpanFromContext(ctx).TracerProvider().Tracer("tracer")
	ctx, span := tracer.Start(ctx, "span")
	defer span.End()
	/* ... */
}

When using the instrumentation of the global trace API, this span will not be recorded.

Describe the solution you'd like

The span created in this above code should be captured by auto-instrumentation. Meaning the nonRecordingSpan should be instrumented.

@MrAlias MrAlias added the enhancement New feature or request label Jul 30, 2024
@MrAlias MrAlias moved this from Todo to In Progress in Go Auto Instrumentation: Beta Dec 10, 2024
@MrAlias MrAlias self-assigned this Dec 10, 2024
@MrAlias MrAlias moved this from In Progress to Todo in Go Auto Instrumentation: Beta Dec 10, 2024
@MrAlias MrAlias removed their assignment Dec 10, 2024
MrAlias added a commit to MrAlias/opentelemetry-go-instrumentation that referenced this issue Jan 6, 2025
The span embedded within a context will be from `auto/sdk`. This means
that open-telemetry#974 is resolved by this change.
@MrAlias MrAlias moved this from Todo to In Progress in Go Auto Instrumentation: Beta Jan 6, 2025
@MrAlias MrAlias self-assigned this Jan 6, 2025
@MrAlias MrAlias added this to the v0.20.0-alpha milestone Jan 6, 2025
MrAlias added a commit that referenced this issue Jan 7, 2025
* Enable auto/sdk for the OTel global probe

Set the auto/sdk flag in the global API to true when newSpan is called.

* Update the otelglobal e2e test

Verify the auto/sdk probe is being used.

* Add a changelog entry

* Unload the newSpan probe after first call

* Update internal/pkg/instrumentation/bpf/go.opentelemetry.io/otel/traceglobal/probe.go

* Add package constraints to otelglobal uprobes

* Remove converter.uprobeNewStartMu

* Test SpanFromContext is also supported

The span embedded within a context will be from `auto/sdk`. This means
that #974 is resolved by this change.
@github-project-automation github-project-automation bot moved this from In Progress to Done in Go Auto Instrumentation: Beta Jan 7, 2025
@MrAlias
Copy link
Contributor Author

MrAlias commented Jan 10, 2025

This was only partially resolved by #1405. If a global span is already active in the context this will work, as demonstrated in #1405. However, if there is no active span a no-op span is still created and is not instrumented.

We need to provide the same enablement logic as the global API for the trace.SpanFromContext function.

@MrAlias MrAlias reopened this Jan 10, 2025
@MrAlias MrAlias moved this from Todo to In Progress in Go Auto Instrumentation: Beta Jan 10, 2025
@MrAlias
Copy link
Contributor Author

MrAlias commented Jan 10, 2025

My initial idea was to update the noopSpan.TracerProvider method:

https://github.com/open-telemetry/opentelemetry-go/blob/784638358b3d39412703a13fe0bd6aa0908ffe45/trace/noop.go#L84-L85

Similar to open-telemetry/opentelemetry-go#5920 we could call a private method with a *bool. For example:

--- a/trace/noop.go
+++ b/trace/noop.go
@@ -6,6 +6,7 @@ package trace // import "go.opentelemetry.io/otel/trace"
 import (
        "context"

+       "go.opentelemetry.io/auto/sdk"
        "go.opentelemetry.io/otel/attribute"
        "go.opentelemetry.io/otel/codes"
        "go.opentelemetry.io/otel/trace/embedded"
@@ -82,4 +83,22 @@ func (noopSpan) AddLink(Link) {}
 func (noopSpan) SetName(string) {}

 // TracerProvider returns a no-op TracerProvider.
-func (noopSpan) TracerProvider() TracerProvider { return noopTracerProvider{} }
+func (s noopSpan) TracerProvider() TracerProvider {
+       return s.tracerProvider(autoInstEnabled)
+}
+
+// autoInstEnabled defines if the auto-instrumentation SDK is enabled.
+//
+// The auto-instrumentation is expected to overwrite this value to true when it
+// attaches to the process.
+var autoInstEnabled = new(bool)
+
+// tracerProvider return a noopTracerProvider if autoEnabled is false,
+// otherwise it will return a TracerProvider from the sdk package used in
+// auto-instrumentation.
+func (noopSpan) tracerProvider(autoEnabled *bool) TracerProvider {
+       if *autoEnabled {
+               return sdk.TracerProvider()
+       }
+       return noopTracerProvider{}
+}

This will not work because it will create an import cycle.

@MrAlias
Copy link
Contributor Author

MrAlias commented Jan 14, 2025

copying the auto/sdk to some go.opentelemetry.io/otel/trace/internal package would also have the same import cycle issue.

@MrAlias MrAlias moved this to In progress in Go Auto Instrumentation: GA Jan 22, 2025
@MrAlias MrAlias removed this from the v0.20.0-alpha milestone Jan 22, 2025
@MrAlias MrAlias added this to the v0.21.0 milestone Feb 3, 2025
MrAlias added a commit to open-telemetry/opentelemetry-go that referenced this issue Feb 4, 2025
#6203)

This copes the `go.opentelemetry.io/auto/sdk` package into the
`go.opentelemetry.io/otel/trace` package. This is done to avoid package
import cycles and still provide an auto-instrumentable SDK (see
open-telemetry/opentelemetry-go-instrumentation#974).

## Overview of changes

The code copied is updated with the following changes. The over-all goal
is to ensure none of this is exported and follows the `auto/sdk` as
close as possible to help maintenance.

### `trace/auto.go`

Consolidation of the following into a single file:

-
https://github.com/open-telemetry/opentelemetry-go-instrumentation/blob/aea085dd2a3640630ac07a2187cbda9d15d2dd00/sdk/tracer_provider.go
-
https://github.com/open-telemetry/opentelemetry-go-instrumentation/blob/aea085dd2a3640630ac07a2187cbda9d15d2dd00/sdk/tracer.go
-
https://github.com/open-telemetry/opentelemetry-go-instrumentation/blob/aea085dd2a3640630ac07a2187cbda9d15d2dd00/sdk/span.go
-
https://github.com/open-telemetry/opentelemetry-go-instrumentation/blob/aea085dd2a3640630ac07a2187cbda9d15d2dd00/sdk/limit.go

Has the following changes:

- `func TracerProvider()` renamed to `newAutoTracerProvider`
- `type tracerProvider struct` renamed to `autoTracerProvider`
- `type tracer struct` renamed to `autoTracer`
- `type span struct` renamed to `autoSpan`
- Lint issues addressed based on this repositories configuration (these
changes are being back-ported upstream)

### `trace/auto_test.go`

Consolidation of the following into a single file:

-
https://github.com/open-telemetry/opentelemetry-go-instrumentation/blob/aea085dd2a3640630ac07a2187cbda9d15d2dd00/sdk/tracer_provider_test.go
-
https://github.com/open-telemetry/opentelemetry-go-instrumentation/blob/aea085dd2a3640630ac07a2187cbda9d15d2dd00/sdk/tracer_test.go
-
https://github.com/open-telemetry/opentelemetry-go-instrumentation/blob/aea085dd2a3640630ac07a2187cbda9d15d2dd00/sdk/span_test.go
-
https://github.com/open-telemetry/opentelemetry-go-instrumentation/blob/aea085dd2a3640630ac07a2187cbda9d15d2dd00/sdk/limit_test.go

Has the following changes:

- Renames in `trace/auto.go` are applied here
- Lint issues addressed based on this repositories configuration (these
changes are being back-ported upstream)

### `trace/internal/telemetry`

Copied from
https://github.com/open-telemetry/opentelemetry-go-instrumentation/tree/aea085dd2a3640630ac07a2187cbda9d15d2dd00/sdk/internal/telemetry

- Pacakge vanity URLs added
- Lint issues addressed based on this repositories configuration (these
changes are being back-ported upstream)
- Use of the package name has been updated

#### `trace/internal/telemetry/test`

Copied from
https://github.com/open-telemetry/opentelemetry-go-instrumentation/tree/aea085dd2a3640630ac07a2187cbda9d15d2dd00/sdk/internal/telemetry/test

- Module name updated
- Documentation updated with new package name
- Testing values updated with new package name

---------

Co-authored-by: Ron Federman <[email protected]>
@MrAlias
Copy link
Contributor Author

MrAlias commented Feb 4, 2025

open-telemetry/opentelemetry-go#6203 has merged. We can no instrument that copy of the SDK to support this.

@MrAlias
Copy link
Contributor Author

MrAlias commented Feb 5, 2025

Blocked by either a release of opentelemetry-go or the resolution of #1753. Currently, when trying to test the added probe the comparison of 1.34.1-0.20250205150531-85fab8be9320 > 1.34.0 is returning false which means the probe package constraint will not work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: In progress
Development

Successfully merging a pull request may close this issue.

1 participant