fix: enable async hooks manager in node tracer close #226

[vmarchaud]

cc @OlivierAlbertini

[codecov-io]

Codecov Report

Merging #232 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #232      +/-   ##
==========================================
- Coverage   98.79%   98.74%   -0.05%     
==========================================
  Files          55       55              
  Lines        2161     2155       -6     
  Branches      151      150       -1     
==========================================
- Hits         2135     2128       -7     
- Misses         26       27       +1

Impacted Files	Coverage Δ
...ckages/opentelemetry-node-tracer/src/NodeTracer.ts	100% <100%> (ø)	⬆️
.../opentelemetry-node-tracer/test/NodeTracer.test.ts	100% <100%> (ø)	⬆️
...telemetry-node-tracer/src/instrumentation/utils.ts	89.47% <0%> (-6.36%)	⬇️
...metry-node-tracer/src/instrumentation/constants.ts	100% <0%> (ø)	⬆️
...try-node-tracer/test/instrumentation/utils.test.ts	100% <0%> (ø)	⬆️
...e-tracer/test/instrumentation/PluginLoader.test.ts	100% <0%> (ø)	⬆️

[OlivierAlbertini]

LGTM

[mayurkale22]

Somehow NodeTracer tests are not running, see below snap (Same thing I observed locally) ->

Any idea?

[OlivierAlbertini]

Somehow NodeTracer tests are not running, see below snap (Same thing I observed locally) ->

Any idea?

Yeah I got the same weird situation with the http-plugin, it's when you put test files in deeper folders... So if you put NodeTracer.test.ts inside instrumentation folder, it will run. It should also work if you put NodeTracer.test.ts inside a folder (not instrumentation only)

[OlivierAlbertini]

Fix NodeTracer.test.ts file. #233

[mayurkale22]

Yeah I got the same weird situation with the http-plugin, it's when you put test files in deeper folders... So if you put NodeTracer.test.ts inside instrumentation folder, it will run.

Yes, right. One option is to remove folder hierarchy in tests and other is to allow multiple paths in test script ex: nyc ts-mocha -p tsconfig.json test/**/*.ts test/*.ts.

[mayurkale22]

This is what I found: mochajs/mocha#3115 and looks to be working in our scenario. Will open PR to fix it.

[OlivierAlbertini]

@vmarchaud if you rebase (#233) you should see NodeTracer tests
/cc @mayurkale22

[mayurkale22]

LGTM

[mayurkale22]

@vmarchaud Could you please fix the build?

[vmarchaud]

@mayurkale22 should be good now

[mayurkale22]

NodeTracerConfig same as BasicTracerConfig except one mandatory attribute. What's the motivation here? I proposed to keep ScopeManager optional attribute in #194. If we do that then we don't need to create NodeTracerConfig interface.

[vmarchaud]

The problem is i cant extend the BasicTracerConfig to tell scopeManager is optional so i'm forced to make a new interface.
I agree that it should be optional and the BasicTracer should create a NoopScopeManager if none is used, if we agree we should open another PR