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

docs: add contributing guidelines for new instrumentations #2259

Conversation

pichlermarc
Copy link
Member

Which problem is this PR solving?

It has become clear that adding instrumentation packages to this repository puts a lot of strain on the OpenTelemetry JavaScript SIG.

Currently Triagers, Approvers and Maintainers of this repository need to keep up-to-date with 40+ different packages and frameworks, used across both the browser and Node.js, sometimes even other, officially unsupported runtimes. All of these packages are being constantly updated and improved and therefore the stream of required maintainance work is never-ending. As a reviewers, SIG members also have to be familiar with the with the internals of the instrumented package AND how this package is used in the real world. While this would be feasible for a handful of packages, it does not scale to the amount of packages that are being hosted in this repository.

The main goal of the SIG is develop and improve the OpenTelemetry Client libraries (the API, the SDK and the extensions defined by the OTel Specification) and to provide the community with tools to instrument their applications and libraries. Adding and maintaining more and more instrumentation poses a significant threat to the overall health of the project.

This PR adds a guideline largely copied from the one the Go SIG has used over the past few years. It provides guidance on where new instrumentation should live:

  • in the same package that's being instrumented ("native instrumentation"), via a direct dependency on @opentelemetry/api which is stable and is expected to stay stable for the foreseeable future.
  • as a separate package, similar to the ones in this repository, provided by either by the authors of the instrumented package, or by community members that wish to instrument that package, but not hosted on this repository
  • only if none of these options are not possible it should be hosted here.

Copy link

codecov bot commented Jun 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.29%. Comparing base (dfb2dff) to head (086a852).
Report is 163 commits behind head on main.

Current head 086a852 differs from pull request most recent head 6ae88f3

Please upload reports for the commit 6ae88f3 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2259      +/-   ##
==========================================
- Coverage   90.97%   90.29%   -0.69%     
==========================================
  Files         146      147       +1     
  Lines        7492     7244     -248     
  Branches     1502     1502              
==========================================
- Hits         6816     6541     -275     
- Misses        676      703      +27     

see 56 files with indirect coverage changes

@pichlermarc pichlermarc marked this pull request as ready for review June 6, 2024 14:41
@pichlermarc pichlermarc requested a review from a team June 6, 2024 14:41
CONTRIBUTING.md Outdated Show resolved Hide resolved
If possible, OpenTelemetry instrumentation should be included in the instrumented package.
This will ensure the instrumentation reaches all package users, and is continuously maintained by developers that understand the package.

If instrumentation cannot be directly included in the package it is instrumenting, it should be hosted in a dedicated public repository owned by its maintainer(s).
Copy link

@maorleger maorleger Jun 7, 2024

Choose a reason for hiding this comment

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

This is what we (the Azure SDK) team do today with our azure sdk instrumentation package

What is not clear to me, and I would love to hear from you all, is how would I then onboard our (separately hosted, deployed, and versioned) instrumentation to the auto-instrumentation-node metapackage?

looking at the code it sounds trivial:

Is that what you'd expect to see? If not, is there any documentation for onboarding to the metapackage? I'm very interested in seeing us added there and would prefer to use our existing package (sounds like y'all prefer that as well).

Caveat is we need to export the class at the top level (today we only export a factory function), but that's an easy change to make 😄

Copy link
Member Author

@pichlermarc pichlermarc Jun 12, 2024

Choose a reason for hiding this comment

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

We'd do not plan depend on thrid-party instrumentations from an OpenTelemetry metapackage. That's mainly for security reasons (I'm not worrying about any Azure packages, but there might be third-parties requesting this treatment that we're not so confident about).

I'm currently prototyping a way to

  • auto-load instrumentations that are present in node_modules as defined by an environment variable. Users would have to explicitly opt-in to the thrid-party instrumentation instead of it being loaded by default, but that's an acceptable trade-off IMO. Essentially one would be able to set something like OTEL_NODE_LOAD_INSTRUMENTATIONS=http,grpc,npm:@azure/opentelemetry-instrumentation-azure-sdk and that would load the http/grpc instrumentations (if present in node_modules) and @azure/opentelemetry-instrumentation-azure-sdk (also only if present)
  • generate code for a getNodeAutoInstrumentations (or similar) function. The code-generation can run as a build step and uses the same environment variable on run-time to control which instrumentations are active. This is intended to help with bundled apps and any cases where loading from node_modules is not an option (that's less interesting for instrumentations than it is for propagators, resource detectors, etc as require is gone in bundled apps). The build step would generate code that imports the instrumentation based on a config file.

I'm not far enough along to figure out how this would work for the OTel Operator, but I'm fairly confident that we can find a solution for this as well. These tools would basically replace the functionality of the auto-instrumentation metapackage. As far as I understand, the reason that metapackage exists today is that we did not have such functionality before.

Choose a reason for hiding this comment

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

Thanks for the information! I understand that the auto-instrumentation metapackage may not be needed in the future however it is still the recommended approach today and I'd like to get our instrumentation on there. The guidance is a bit conflicting though - I am reading that:

  • OTel doesn't want to pull in 3rd party instrumentations into the OTel metapackage
  • OTel doesn't want folks to add 3rd party instrumentations into this repo (that is the 3rd option in order of preference) because of the maintenance burden

I also asked this in #2238 if you prefer to engage somewhere but a set of clear next steps would be much appreciated!

If not, I can also open a PR with what I think makes sense and let y'all provide feedback that way - I'd rather know what you prefer beforehand though

Just to be concrete: I am interested in getting the Azure SDK instrumentation into the @opentelemetry/auto-instrumentation-node metapackage. I don't particularly care which approach I need to take and am happy to follow your guidance, but am confused about which path y'all prefer.

Thanks again!

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.

6 participants