opentelemetry-js icon indicating copy to clipboard operation
opentelemetry-js copied to clipboard

[instrumentation] instrumentation base stabilization plan

Open pichlermarc opened this issue 1 year ago • 3 comments
trafficstars

Description

The @opentelemetry/instrumenation package is the base package used for all instrumentations. To reduce the maintenance burden for instrumentation authors which currently need to deal with possible breaking changes as well as users that use the instrumentation registration mechanism, we should promote the @opentelemetry/instrumenation package to stable.

Currently the following issues prevent stabilization:

Logs API stability:

Types from the logs API are used in the public interface and may break type compatibility when using different versions of the instrumentation base in the same app. Therefore we need to hold off on marking the @opentelemetry/instrumentation package as stable until the Logs API is integrated into the @opentelemetry/api package as a stable component OR we come up with a way to have experimental features in @opentelemetry/instrumentation, where the stable interface does not depend on the non-stable @opentelemetry/api-logs package.

Option 1 (Logs API stabilization)

This will be addressed by

  • [ ] #4400
  • [ ] TBD(if API was moved to @opentelemetry/api/experimental in #4400, issues to stabilize the API)
    • an approver or maintainer will create the issue if this is the case

Option 2 (Experimental features in `@opentelemetry/instrumentation)

This will be addressed by

  • [ ] #4839
  • [ ] (TBD) issue to implement the previously researched and accepted solution

@types/shimmer being part of the public API

@types/shimmer is currently part of the public API. We should avoid having a third-party dependency be part of the public API of this package.

This will be addressed by:

  • [x] #4837

ESM instrumentation:

We currently offer an instrumentation customization hook for ESM in Node.js which uses import-in-the-middle. As of writing, the customization hooks concept is not yet stable in Node.js, but it is already available as a RC.

We should therefore explicitly document one or multiple minimum supported Node.js versions / version ranges (especially with regards to Node.js LTS versions) based on the stability of the customization hook in that Node.js version. If no versions are stable yet declare the provided customization hook as fully experimental. Update versions as needed.

We should also document that direct usage of import-in-the-middle/hook.mjs may cease to work in the future and is purely experimental to prevent being locked into using import-in-the-middle until the next major version, should we decide to move on from using that package for ESM instrumentation in the future and implement our own version of the customization hook at @opentelemetry/instrumentation/hook.mjs

We should also update documentation on how to set this customization hook when using the Node.js binary, be it via --import or otherwise, and explicitly declare all other ways that may be introduced in future Node.js versions as not directly supported (we may declare them as supported in a new minor version).

This will be addressed by

  • [ ] #4845

Public API review:

We should replace any occurrence of export * with explicit exports and and review the API surface for any types and functions that we may be able to remove. We should also do a final review of the state of the @opentelemetry/instrumenation package after the improvements mentioned above, and ensure that the package's API is up to our standards.

This will be addressed by

  • (TBD) issue to review the API surface
    • A maintainer/approver will create this issue once the above tasks are complete.
  • (TBD) depending on review of the API surface, any follow-up issues to deal with any shortcomings we've identified.
    • The person(s) working on the API surface review will create these follow-up issues

pichlermarc avatar Mar 26 '24 17:03 pichlermarc

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions[bot] avatar May 27 '24 06:05 github-actions[bot]

update: applied in https://github.com/open-telemetry/opentelemetry-js/pull/4847

~~we should probably also consider if we want to remove instrumentationDescription of the public api Instrumentation interface. I don't see any reason for it to exist in code if we choose to walk the path https://github.com/open-telemetry/opentelemetry-js-contrib/pull/2292. It is not used anywhere and one can already consume it from the package.json of each instrumentation package. related: https://github.com/open-telemetry/opentelemetry-js/issues/4725~~

blumamir avatar Jun 27 '24 15:06 blumamir

one more breaking issue is making the instrumentation base _config property private, and so config interactions are only via the getter and setter:

Here is my opinion. If we already have a public API to access the field I think we should leverage that for accessing even in subclasses.

See https://github.com/open-telemetry/opentelemetry-js/issues/4668 currently blocked by https://github.com/open-telemetry/opentelemetry-js-contrib/pull/2289

blumamir avatar Jul 17 '24 15:07 blumamir