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

Enable auto/sdk in otel global #1405

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Dec 10, 2024

Resolve #1399
Resolve #1400
Resolve #974

This includes a probe.Uprobe refactor. Instead of passing back links when the uprobe loads, the uprobe itself now hold these values. When its Close method is called it will, itself, call each of these close methods.

This redesign is used so that the newSpan uprobe can be closed prior to the others (which are closed by the Manager ).

@MrAlias

This comment was marked as resolved.

@MrAlias MrAlias force-pushed the otel-global-autosdk branch 2 times, most recently from bc3fd1e to 2cc75b0 Compare December 10, 2024 22:22
@MrAlias MrAlias force-pushed the otel-global-autosdk branch from 0fa0bd8 to 555a184 Compare December 11, 2024 00:59
@MrAlias

This comment was marked as outdated.

@MrAlias MrAlias force-pushed the otel-global-autosdk branch from 555a184 to dab479a Compare December 12, 2024 22:23
@MrAlias MrAlias force-pushed the otel-global-autosdk branch 5 times, most recently from d5356ef to 89c0867 Compare December 19, 2024 21:21
@MrAlias MrAlias changed the title [PoC] Enable auto/sdk in otel global Enable auto/sdk in otel global Dec 19, 2024
@MrAlias MrAlias force-pushed the otel-global-autosdk branch 5 times, most recently from b25ff44 to 30e7015 Compare December 20, 2024 16:16
@MrAlias MrAlias marked this pull request as ready for review December 20, 2024 16:31
@MrAlias MrAlias requested a review from a team as a code owner December 20, 2024 16:31
@MrAlias MrAlias added this to the v0.20.0-alpha milestone Dec 20, 2024
Copy link
Contributor

@RonFed RonFed left a comment

Choose a reason for hiding this comment

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

It's very cool to see it all come together,
Left some comments but it looks great.

@@ -365,50 +371,76 @@ type Uprobe struct {
// function specified by Sym. If ReturnProbe is empty, no eBPF program will be attached to the return of the function.
ReturnProbe string
DependsOn []string

closers atomic.Pointer[[]io.Closer]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not []io.Closer?
What are the concurrent scenarios we are trying to protect against?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Uprobe.Close is called by the Manager which can shutdown asynchronously. For example, this can occur when the context passed to the new Instrumentation is canceled asynchronously1. This is to prevent that data-race.

Footnotes

  1. https://github.com/open-telemetry/opentelemetry-go-instrumentation/blob/aeb95422568a2e2901a02620527e526161936321/cli/main.go#L87-L90

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Do you think we can use sync.Once instead? I think it might be more readable

Copy link
Contributor Author

@MrAlias MrAlias Jan 6, 2025

Choose a reason for hiding this comment

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

Using sync.Once would mean we need two fields here. One for the sync.Once and the other to hold the closer slice. I think having a single field is cleaner. It does not require a user to understand the implicit coupling between two fields and avoids the error case when they are not correctly handled.

Alternatively, we could use a sync.OnceFunc as a single field. However, this would leave a data-race when trying to replace the "swap" functionality. We would want to check if the "close" field is non-nil before assigning to it in load. This would need a semaphore to ensure no data-race, and again mean we require two fields.

It may also be tempting to not implement the "swap" functionality. That way we would just assign blindly on load and hope for the best on close (meaning there would be a data-race there). However, data-race asside, that would mean we no longer support instrumentation shutdown and then restarting, or probe unloading and reloading.

We could also use atomic.Value here. That was explored, but decided against due to the lack of type safety and the inability to set a value of nil (representing a state of stopped) without a panic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also add package constraints on the auto sdk probes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we want to always add uprobes for auto/sdk. If that package is loaded, it may be because the user has done so explicitly and we don't want to not instrument it because of the version of otel. It also doesn't add runtime overhead if it is not used given the uprobe break-point is never triggered.

@@ -365,50 +371,76 @@ type Uprobe struct {
// function specified by Sym. If ReturnProbe is empty, no eBPF program will be attached to the return of the function.
ReturnProbe string
DependsOn []string

closers atomic.Pointer[[]io.Closer]
Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Do you think we can use sync.Once instead? I think it might be more readable

@MrAlias MrAlias force-pushed the otel-global-autosdk branch from 955785e to 4b55aa7 Compare January 6, 2025 19:25
The span embedded within a context will be from `auto/sdk`. This means
that open-telemetry#974 is resolved by this change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
2 participants