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

WIP: add a new plugin base decoupled from the sdk #1449

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Aug 19, 2020

This new plugin base is completely decoupled from the SDK and relies only on the API. After #1448 plugins using this base class will be able to be fully standalone, and will support instrumenting modules very early in the load process, even before the tracer provider is set up.

  • Solves load order issue
  • Solves yarn pnp issue by using a more direct dependency model
  • Removes need for plugin manager
  • One instrumentation can patch multiple modules
  • Can use different plugins for different versions of a module
  • Supports tracers and meters

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

overall lgtm, one question is this only for node ? If so maybe name should have node word otherwise the semver usage should be moved to platform folder

this._meter = meterProvider.getMeter(this.instrumentationName, this.instrumentationVersion);
}

protected get tracer(): Tracer {
Copy link
Member

Choose a reason for hiding this comment

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

we are basically not using get/set approach in our sdk so would rather have getTracer, getMeter to just unify all , WDYT ?

@dyladan
Copy link
Member Author

dyladan commented Sep 9, 2020

overall lgtm, one question is this only for node ? If so maybe name should have node word otherwise the semver usage should be moved to platform folder

So far I have been focusing on node but I hope that the module will eventually be useful for both web and node.

Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

looks fine 👍

Another question that comes though my mind is since the API is changed a bit, how do handle old plugin loading AND new ones ? Or there will be a breaking change here ?

@dyladan
Copy link
Member Author

dyladan commented Sep 9, 2020

looks fine 👍

Another question that comes though my mind is since the API is changed a bit, how do handle old plugin loading AND new ones ? Or there will be a breaking change here ?

The new plugin loader depends on API only, so it doesn't actually depend on any plugin load mechanism at all so the old plugins will still work. I think for now this will just be an alternative plugin load mechanism, then when all plugins have been migrated we can remove the old plugin loader.

@obecny
Copy link
Member

obecny commented Dec 14, 2020

I think this can be closed already @dyladan

@dyladan dyladan closed this Dec 16, 2020
@dyladan dyladan deleted the instrumentation-base branch December 16, 2020 13:29
pichlermarc added a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
* Add process metrics in @opentelemetry/host-metrics

* Lint

---------

Co-authored-by: Marc Pichler <[email protected]>
Co-authored-by: Haddas Bronfman <[email protected]>
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.

3 participants