pass stats instance to plugin loader #386
Conversation
Codecov Report
@@ Coverage Diff @@
## master #386 +/- ##
==========================================
- Coverage 94.7% 94.65% -0.06%
==========================================
Files 150 150
Lines 9785 9594 -191
Branches 739 720 -19
==========================================
- Hits 9267 9081 -186
+ Misses 518 513 -5
Continue to review full report at Codecov.
|
| @@ -21,7 +21,6 @@ | |||
| }, | |||
| "dependencies": { | |||
| "@opencensus/exporter-zipkin": "^0.0.9", | |||
| "@opencensus/instrumentation-http": "^0.0.9", | |||
There was a problem hiding this comment.
Out of curiosity, why is this no longer needed?
There was a problem hiding this comment.
There are two ways to instrument supported plugins. 1) Manual approach: include respective dependency and use plugin.enable(...) interface. 2) Allow the library to instrument plugins automatically, for this start the trace instance as the first module in your application.
The changed the HTTP example to work with second (I think recommended) approach.
| @@ -49,9 +51,10 @@ export class PluginLoader { | |||
| * Constructs a new PluginLoader instance. | |||
| * @param tracer The tracer. | |||
| */ | |||
| constructor(logger: Logger, tracer: Tracer) { | |||
| constructor(logger: Logger, tracer: Tracer, stats: Stats|undefined) { | |||
There was a problem hiding this comment.
Why not just have it be stats?: Stats?
| @@ -140,8 +142,14 @@ export class PluginLoader { | |||
| try { | |||
| const plugin: Plugin = require(moduleName as string).plugin; | |||
| this.plugins.push(plugin); | |||
| return plugin.enable( | |||
| exports, this.tracer, version, moduleConfig, basedir); | |||
| if (this.stats) { | |||
There was a problem hiding this comment.
Changing the type signature above to make stats optional would also allow collapsing down the two branches of this if
| @@ -94,7 +94,7 @@ describe('Plugin Loader', () => { | |||
| }); | |||
|
|
|||
| it('should load a plugin and patch the target modules', () => { | |||
| const pluginLoader = new PluginLoader(log, tracer); | |||
| const pluginLoader = new PluginLoader(log, tracer, undefined); | |||
There was a problem hiding this comment.
These changes to pass undefined would also be unnecessary with it being optional.
mayurkale22
force-pushed
the
automatic_tracing
branch
from
3cf43bf
to
508e6ce
Compare
mayurkale22
force-pushed
the
automatic_tracing
branch
from
508e6ce
to
59b2c28
Compare
mayurkale22
force-pushed
the
automatic_tracing
branch
from
59b2c28
to
9764cc6
Compare
Allow a way to pass
statsinstance toPluginLoaderclass, this will help users to collect stats for HTTP and GRPC plugin with automatic tracing.Also, I have updated the HTTP instrumentation example to capture traces without manually enabling the plugin. I think this is the recommended and straightforward way for users to enable tracing on supported plugins/frameworks.