-
Notifications
You must be signed in to change notification settings - Fork 91
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 manual instrumentation support #352
Comments
If the following is true:
Then I think this issue would be only about updating the documentation and tests. |
Hey @pellared - thanks for creating the issue. Yes, we agree that the agent should support mixing automatic and manual instrumentation. As you found with .NET's implementation, without it the usefulness of this project is very limited as many users will want to graduate to a higher level of instrumentation that would require manually creating spans to capture additional business logic. I see this as hard requirement for this project to move into beta because it could affect some core functionality within the current agent. |
What is the problem here? Just that the instrumentation currently sends telemetry to two different pipelines ( |
Yes. I think that the Go automatic instrumentation should set the global providers to decrease the amount of coding required to add additional custom telemetry. For reference, this is what https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation does. See https://opentelemetry.io/docs/instrumentation/net/getting-started/ |
I started implementing some of the required functionality for this issue and would like to hear your thoughts. I'm using the above roll-dice example for my testing. Currently I added probes to the following functions: I looked at instrumenting Another difference is that SetAttributes does nothing for So I guess the main question is how important it is to support |
I don't think we should be overwriting the For example, if a user has instrumented their app directly with a This would also be an overreach. Observability backends are designed to solve this problem. If we can have the From some ad-hoc testing, it appears we do not currently inject the span context into the context returned by Similarly for the |
I think that the Go Automatic Instrumentation should "by default" set the global providers and use them as well. On top of it, we should add a possibility to:
From my experience based on https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation 99% of use cases under following categories:
|
@MrAlias
While both options have pros and cons, I personally think that choosing to process manual spans with eBPF probes is friendlier for users, easier to debug and develop, and is less mentally complex than the alternative. |
I think we can take inspiration from the Java auto instrumentation. |
@pellared how do you set the global provider in another process? |
@blumamir the eBPF instrumentation will never live within the same process as the running code. |
I am not sure about the precise definitions here, but what I meant to say is that a single process will generate telemetry signals (like spans) from 2 different SDKs. |
Notes from SIG meeting:
Action items
|
Catching up on this thread after reading through the proposal in #523. I'm concerned that setting up uprobes on the Go SDK's Start & End funcs effectively means that we're duplicating instrumentation (once in the SDK, then again in the instrumentation process). Have we considered or attempted to construct trace & metrics providers and inject them into the process? Maybe we could detect a given provider has not been configured during init. This would happen once and then the auto instrumentation agent wouldn't need to interact with every span that's created in the target process with the Go SDK. |
@pellared the proposed solution to (2) is adding support to capture from the default global implementation. It will not capture telemetry from any SDK registered with the global implementation, nor any default OTel SDK. Does this address (2) in your understanding? |
Wouldn't this violate the Go memory management requirements? We would be using types outside of the Go allocated memory space within a Go process. |
SIG meeting: Moving it post-beta. There is no reason to delay the beta release so much. |
Is your feature request related to a problem? Please describe.
One of the key aspects of observability is to have the ability to create instruments which are oriented around the application logic. Without this possibility the automatic instrumentation is more like "monitoring" tool.
Describe the solution you'd like
Add the application developer the possibility to augment the instrumentation in the application so that they can:
For example, let's consider this application which is based on https://opentelemetry.io/docs/instrumentation/go/getting-started/:
Key requirements:
roll
spans anddice.rolls
metrics) is exported recorded and exported via the tracer provider and meter provider that is set up by Go Automatic Instrumentation.So the manual telemetry should be integrated with the telemetry produced by Go Automatic Instrumentation.
As part of this issue it would be also good to update https://github.com/open-telemetry/opentelemetry-go-instrumentation/tree/main/examples/rolldice/main.go with the code above.
Additional context
The need for this feature in .NET Automatic Instrumentation has resulted in complete redesign and code rewrite. Docs for reference: https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/blob/main/docs/manual-instrumentation.md
Without this feature automatic instrumentation would be only a "poor mans" instrumentation as it would not be able to compete with the manual OTel Go SDK approach.
Here are some docs around manual instrumentation in this repository: https://github.com/open-telemetry/opentelemetry-go-instrumentation/blob/main/docs/design/manual-instrumentation.md
The text was updated successfully, but these errors were encountered: