-
Notifications
You must be signed in to change notification settings - Fork 94
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
Donation proposal: OpenTelemetry Go Automatic Instrumentation #2
Comments
@edeNFed thank you for the proposal. I believe this can be a very valuable addition to OpenTelemetry. I had a quick glance at the project and it looks very promising. I am going to add this to today's TC meeting agenda (good timing!). A few quick questions to help us get started:
A few more questions (quotes from the "How it works" doc):
Do you regenerate the offsets every time a new version of a supported library is released? What happens if the version of library used is unsupported? Does instrumentation detect it and bail out?
How reliable is this detection? Is it possible to miss a return statement? What if for example the function exits by panic and not by return? If missed does the span remain forever unfinished? Are these analysis dependent on the platform the machine code is in? Does it work on x86-64 only or other platforms too?
I assume this is dependent not only on the compiler version but also the target platform, so each specific combination may require a different analyzing code. That sounds like quite a lot of targets to support and keep maintaining. What has been your experience with this? How cumbersome it is? |
Hi @tigrannajaryan, thank you for the quick response.
Let me know if I can help with anything else 😄 |
@edeNFed thanks a lot for the answers, they are very helpful. The TC has discussed the donation. We believe that it indeed can be a very valuable addition to OpenTelemetry. We have 2 primary concerns:
Both of these are on your roadmap already, which is great. It would be very useful if you could provide more details about what you think the solutions will look like. Are there any design documents on these topics? We do not necessarily require that donations are complete and done before we accept them. However we do need certain level of confidence that important OpenTelemetry functionality is implementable, even if some of the implementation work happens after the donation. The TC believes the above 2 topics are important for an OpenTelemetry instrumentation. We would like to learn a bit more about how you intend to solve these before we are ready to move forward. |
Makes sense. |
Hi @tigrannajaryan, I believe those documents should give a good idea regarding how we are going to implement both features. |
Thanks @edeNFed I will add this to the next week's TC meeting agenda to review the proposal with the new design documents again. |
I had a quick look at the Context Propagation proposal. The proposal uses goroutine ids to associate incoming and outgoing requests. AFAIK there is no official way to obtain goroutine ids, in fact this is actively discouraged by Go language documentation. I know there are some hacky ways to obtain the goroutine id but I am not aware of any production quality implementations to get the goroutine id. IMO this may be a deal breaker. I wonder what @open-telemetry/go-approvers, @open-telemetry/collector-approvers, @open-telemetry/collector-contrib-approvers think. |
Yuri had a similar opinion, reposting what I answered on the other issue: Regarding depending on goroutine ids: The main objection that I saw is that Go's internal structures like |
Context: I stumbled over this issue following up on some notes from OpenTelemetry Day in Austin; we had a well-attended eBPF breakout session there. See https://photos.app.goo.gl/EboFNpkGz41EE6ve6 for some notes from that session. @tigrannajaryan the FAQ discourages the use of goroutine IDs by developers while designing and writing code. It lists good arguments there. It does not seem to argue against using them for automated extraction of information at runtime. Carrying a small lookup table for well-known offsets as linked to by @edeNFed seems to be a reasonable and lightweight trade-off. The one constraint it introduces is that the autoinstrumentation needs to be at least as current as the Go version used for compilation, which seems to be another reasonable trade-off. |
There is another approach to avoid tracking goroutine ids (suggested by @tedsuo):
Overall I think this approach is less hacky but may be more error-prune (from the user perspective). If the TC believes this is a better way, I'm also good with this implementation. |
It may also be less error prone in certain situations. My personal opinion is that |
👍🏻 Agree. Please let me know if there are more pending questions. |
Regrading manual + automatic instrumentation coordination, can you update with what you'd do for Metrics (and soon, Logs) APIs? I assume it'd be similar but would be good to write down. |
@jsuereth sure here are my initial thoughts about coordination with metrics/logs manual instrumentation: Metrics: Logs: |
Understand the point. My main concerns are as follows:
It sounds like yes, but for (2) wantedto make sure, e.g. Auto + manual instrumetnation use the same exporters/buffering/etc. |
Yes this is the plan. I will add it to the roadmap and will extend the design documents to also describe metrics and logs. |
+1 on not using the goroutine ID as the source of operation identity. The important part of tracing instrumentation is the ability to connect spans into a unified trace that represents the operation workflow. I'm guessing there is a plan to introspect a context to determine if there is an active span present that can act as a parent, but what happens when there is not? There is a good chance that many orphaned spans will be created if every function that accepts a context is encapsulated in a span, but there is an origination issue if you never start a span because of the absence of a parent. How do you plan to originate traces? @dashpole I know has put some thought into this. I would value his take if he has time to provide it 😄. |
This is not how it works. Orphans are also not a problem, If there is no active span in the context a new span will be created. Basically, it means that the root span of the trace will be the first instrumented function that got invoked in the request. |
Gotcha 👍 Do we have a list of functions this will support? Also, how will library support be expanded? I would be hesitant to add gRPC support, at least initially, as it is not included in the standard library. But it brings up the question about how 3rd party libraries will be supported and how will we separate them (or not?). |
Every implementation of the |
Gotcha, thanks. From a community support perspective, how are we planing to maintain this? Who will comprise the maintainer roles and what groups plan to support the development? |
I guess we should follow the same process that other donations went through. |
@open-telemetry/technical-committee correct me if I'm wrong, but I'm not sure there is a precedence. It looked like other donations were handed off to existing SIGs. I imagined this living in its own repository, like the Java auto-instrumentation. Is that the intention? If so, we might also need a distinct maintainer group from the opentelemetry-go maintainers. I wouldn't want to assume we all have the bandwidth to be a part of this. I would be happy to be a maintainer for this, but would want to ensure others would as well. Similarly for Approvers, we would want to build an initial group that might differ from the opentelemetry-go group. |
.NET auto-instrumentation has it in a similar way: separate SIG + teams from the .NET SDK. |
Is it possible for for an eBPF to modify the argument to a function? I'm not the most familiar with the technology, but I thought the eBPF virtual machine has read-only access to the syscall parameters and return value. |
Yes. Please look at the Context propagation design doc I added a proof of concept for that in the document. |
@edeNFed OpenTelemetry Community guidelines for donations require a timely response, I'm very sorry we have not been able to assemble one until now. The community reviewed this proposal of yours and @pdelewski's #3. We see both of these as technically viable solutions to the auto-instrumentation problem. OpenTelemetry endorses work on auto-instrumentation as a sub-group of each language SIG, and it appears that we have the making of a new OpenTelemetry-Go auto-instrumentation group in these two donation proposals. We like both of these proposals, and we recognize that methods for auto-instrumentation are only part of the larger-problem that includes all aspects of automatic SDK setup and program initialization. From what we can tell, there is not a hard boundary between automatically configuring the Golang SDK and automatically configuring Golang instrumentation. We recommend the creation of an We also expect members of the OpenTelemetry-Go vendor community looking to streamline and automate SDK setup for vendor-specific configuration (or, e.g., those looking for open-telemetry/opentelemetry-specification#1773 for Go), to work together in this new repository. Finally, we look forward to members of the auto-instrumentation group to work with the OpenTelemetry-Go maintainers and Go developers to bridge gaps between built-in diagnostic features and telemetry SDK support. See, for example,
@edeNFed are you willing to take ownership of the code that you propose donating and eventually become a maintainer of the (proposed) new opentelemetry-go-instrumentation repository? |
@jmacd Yes. I agree to take ownership over the proposed code and become a maintainer in the new repository. Thank you for taking the time to review this donation proposal. |
@edeNFed I created https://github.com/open-telemetry/opentelemetry-go-instrumentation. One of the first tasks for you and @pdelewski will be to become members of the OpenTelemetry org and begin sending and reviewing PRs in this repository. I copied the current Go maintainers in as maintainers of this project temporarily; please consider me your sponsor, I will be glad to help review the first steps in this repository as we work to establish you as new a new maintainer. |
I've tranfered this issue. We can close this when the donation has been merged. Thanks, @edeNFed |
Opened #4 |
@edeNFed The go-instrumentation uses https://github.com/keyval-dev/offsets-tracker to generate |
Hi @dineshg13 I totally agree (I mentioned the offset tracker repo in the original donation proposal). Not sure if this is needed as a first step but ultimately I also think this should be owned by the community. |
Thanks @edeNFed , i don't think it is necessary as a first step. We can add it to our roadmap |
Closing issue as PR is merged. |
Description of donated project
This project adds OpenTelemetry instrumentation to Go applications without having to modify their source code.
Instrumentation is done by using eBPF uprobes.
Automatic instrumentation is available for a wide range of Go applications: Go version 1.12 and above, in addition to supporting stripped binaries (compiled with
go build -ldflags "-s -w"
)Instrumented libraries follow the OpenTelemetry specification and semantic conventions to produce standard OpenTelemetry data.
GitHub repos:
https://github.com/keyval-dev/opentelemetry-go-instrumentation
https://github.com/keyval-dev/offsets-tracker (used by this project, can also be donated if needed)
A detailed technical explanation is documented here.
Summary of existing usage of donated project
This project is a core part of keyval's product and production systems.
We are using this instrumentation successfully on many Go applications including multiple popular open-source Go projects.
Benefits to the OpenTelemetry community
OpenTelemetry Adoption
Go is the most popular programming language in the CNCF landscape (see DevStats)
Providing automatic instrumentation for Go applications would allow projects without the resources needed for implementing manual instrumentation to easily adopt OpenTelemetry.
Alignment
Currently, automatic instrumentation exists for Python, .NET, JavaScript, and Java. Our goal is to provide the same level of automatic instrumentation for Go applications. We hope this project will lead the way for automatic instrumentation for other compiled languages such as Rust and C++ which may also be implemented using eBPF.
Security
Running eBPF programs requires elevated privileges. We believe that being part of the OpenTelemetry community would make users more comfortable using this project.
Maintenance plan
Keyval will continue to maintain this project and we welcome the opportunity to work with more contributors.
Multiple developers in the OpenTelemetry community (from Go SDK SIG, Operator SIG & eBPF SIG) and in the eBPF community have expressed interest in contributing to this project.
Our current roadmap contains the following tasks:
Existing software license(s)
This project is licensed under the terms of the Apache 2.0 open source license.
List of trademarks (if any)
None
The text was updated successfully, but these errors were encountered: