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

Manual instrumentation support - Phase 1 #523

Merged
merged 37 commits into from
Dec 7, 2023

Conversation

RonFed
Copy link
Contributor

@RonFed RonFed commented Nov 22, 2023

This PR addresses some of the functionality described in #352 .
The following change adds the ability to integrate with manually created spans via the OpenTelemetry APIs.
Currently the eBPF collects the start and end time, span name and attributes passed by the user via SetAttributes.
This functionality is disabled by default and can be enabled via an env var or passing an Instrumentation option WithGlobal()
Added e2e tests.
This is only a partial implementation, the following are known gaps that will be added in future PRs:

  • Slice attributes.
  • Events
  • Span errors

Next steps until full implementation are described in #352 (comment)

@RonFed RonFed requested a review from a team November 22, 2023 14:19
Copy link
Member

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

Is the intention here that you configure your application with the OTel SDK and out it into non-recording (eg no export) mode then the auto-instrumentation process watches for the tracer starting spans and duplicate them so they can be exported? It kinda feels wasteful as we'll be creating every span twice.

internal/include/go_types.h Outdated Show resolved Hide resolved
internal/include/otel_types.h Outdated Show resolved Hide resolved
internal/include/otel_types.h Outdated Show resolved Hide resolved
@RonFed
Copy link
Contributor Author

RonFed commented Nov 27, 2023

Is the intention here that you configure your application with the OTel SDK and out it into non-recording (eg no export) mode then the auto-instrumentation process watches for the tracer starting spans and duplicate them so they can be exported? It kinda feels wasteful as we'll be creating every span twice.

Initially, I thought of attaching the uprobes to the Otel SDK functions however, this PR attaches them to the API functions. This means that this feature is only relevant for applications that don't have an active SDK set-up (you can see an example in the roll-dice application). These API calls are basically no-ops but the eBPF is using them to collect the user attributes. so if this feature is opted in there shouldn't be a span duplication.

RonFed and others added 2 commits November 27, 2023 15:36
Co-authored-by: Mike Goldsmith <goldsmith.mike@gmail.com>
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

This doesn't seem to be inline with the proposed solutions: #352 (comment)

Provide a TracerProvider implementation from auto-instrumentation that manual instrumentation can explicitly use to say they want their data to go to the same pipeline. This means the auto-instrumentation will control the type that manual instrumentation uses to create, edit, and end spans if they want the data to be sent to the auto-instrumentation pipeline

The proposal was to provide a TracerProvider implementation here that we would detect being used by other processes. The changes proposed in this PR are instrumenting the global implementation from go.opentelemetry.io/otel.

There are going to be serious limitations to this approach given the global from otel provides delegation support (i.e. the nonRecordingSpan or Tracer there will not be called when the user sets a TracerProvider).

I'm not sure this is a viable solution.

@MrAlias
Copy link
Contributor

MrAlias commented Nov 28, 2023

This doesn't seem to be inline with the proposed solutions: #352 (comment)

Provide a TracerProvider implementation from auto-instrumentation that manual instrumentation can explicitly use to say they want their data to go to the same pipeline. This means the auto-instrumentation will control the type that manual instrumentation uses to create, edit, and end spans if they want the data to be sent to the auto-instrumentation pipeline

The proposal was to provide a TracerProvider implementation here that we would detect being used by other processes. The changes proposed in this PR are instrumenting the global implementation from go.opentelemetry.io/otel.

There are going to be serious limitations to this approach given the global from otel provides delegation support (i.e. the nonRecordingSpan or Tracer there will not be called when the user sets a TracerProvider).

I'm not sure this is a viable solution.

This is not addressing point (1) from the linked solutions, it is addressing point (2).

@MrAlias
Copy link
Contributor

MrAlias commented Nov 28, 2023

Talked with @edeNFed in SIG meeting:

This is only meant to address the situation where a user has not registered an SDK with the global and they opt-in with the auto-instrumentation to record this telemetry. This is not intended to capture the telemetry from a default OTel SDK.

instrumentation.go Outdated Show resolved Hide resolved
internal/pkg/instrumentation/manager.go Outdated Show resolved Hide resolved
instrumentation.go Outdated Show resolved Hide resolved
internal/include/otel_types.h Outdated Show resolved Hide resolved
internal/test/e2e/otelglobal/main.go Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
examples/rolldice/main.go Outdated Show resolved Hide resolved
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@RonFed
Copy link
Contributor Author

RonFed commented Dec 3, 2023

@MrAlias Can you please take another look?

internal/include/otel_types.h Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@MrAlias MrAlias merged commit dc3681a into open-telemetry:main Dec 7, 2023
16 checks passed
@MrAlias MrAlias added this to the v0.9.0-alpha milestone Dec 12, 2023
@MrAlias MrAlias mentioned this pull request Dec 12, 2023
@MrAlias MrAlias mentioned this pull request Jul 16, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants