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

fix(api): make failed diag.setLogger not be silent; correct versions in incompat error message

Open trentm opened this issue 2 years ago • 4 comments
trafficstars

This fixes two issues in registerGlobal handling in the API.

make failed diag.setLogger not be silent

First, in the following script the second diag.setLogger fails to register, because API v1.7.0 is incompatible with the already registered API v1.5.0. That is as expected. However, that failure is silent, even though a perfectly functional DiagConsoleLogger() is passed in.

// silent-diag-reg-fail.js
//
// Usage:
//    cd opentelemetry-js/api/
//    npm install --no-save api150@npm:@opentelemetry/[email protected]
//    node silent-diag-reg-fail.js

const api170 = require('.'); // @opentelemetry/[email protected]
const api150 = require('api150'); // npm install --no-save api150@npm:@opentelemetry/[email protected]

api150.diag.setLogger(new api150.DiagConsoleLogger(), api150.DiagLogLevel.DEBUG);
api150.diag.info('hi 1.5.0');

// (1)
api170.diag.setLogger(new api170.DiagConsoleLogger(), api170.DiagLogLevel.DEBUG);
api170.diag.info('hi 1.7.0');

Running this yields:

% node silent-diag-reg-fail.js
@opentelemetry/api: Registered a global for diag v1.5.0.
hi 1.5.0

https://github.com/open-telemetry/opentelemetry-js/blob/f5ef8de1cc92ad22d7d95df6a5f585f9d64ddef1/api/src/internal/global-utils.ts#L33-L59

The registerGlobal(...) is given a diag argument that is used to log the failed register. The issue is here:

https://github.com/open-telemetry/opentelemetry-js/blob/f5ef8de1cc92ad22d7d95df6a5f585f9d64ddef1/api/src/api/diag.ts#L104

When the DiagAPI registers the newLogger, it passes self as that diag argument. self here is an object which proxies .error(), .info(), et al methods through the registered diag global (via getGlobal('diag')). Because the registration fails, getGlobal('diag') returns undefined and the error message is lost.

However, if it instead passes in newLogger, then any registration failures will get logged:

% node silent-diag-reg-fail.js
@opentelemetry/api: Registered a global for diag v1.5.0.
hi 1.5.0
Error: @opentelemetry/api: Registration of version v1.5.0 for diag does not match previously registered API v1.7.0
    at registerGlobal (/Users/trentm/tm/opentelemetry-js2/api/build/src/internal/global-utils.js:41:21)
    at DiagAPI.setLogger (/Users/trentm/tm/opentelemetry-js2/api/build/src/api/diag.js:69:54)
    at Object.<anonymous> (/Users/trentm/tm/opentelemetry-js2/api/silent-diag-reg-fail.js:15:13)
    at Module._compile (node:internal/modules/cjs/loader:1256:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1310:10)
    at Module.load (node:internal/modules/cjs/loader:1119:32)
    at Module._load (node:internal/modules/cjs/loader:960:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:86:12)
    at node:internal/main/run_main_module:23:47

correct versions in incompat error message

Second, notice how that error message has the versions backwards:

Registration of version v1.5.0 for diag does not match previously registered API v1.7.0

The previously registered version was v1.5.0. This PR reverses those versions in the error message:

% node silent-diag-reg-fail.js
@opentelemetry/api: Registered a global for diag v1.5.0.
hi 1.5.0
Error: @opentelemetry/api: Registration of version v1.7.0 for diag does not match previously registered API v1.5.0
    at registerGlobal (/Users/trentm/tm/opentelemetry-js2/api/build/src/internal/global-utils.js:38:21)
...

trentm avatar Nov 11 '23 00:11 trentm

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.20%. Comparing base (4daa264) to head (eec90ea). Report is 213 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4288      +/-   ##
==========================================
- Coverage   92.64%   92.20%   -0.44%     
==========================================
  Files         307      332      +25     
  Lines        9019     9447     +428     
  Branches     1919     2003      +84     
==========================================
+ Hits         8356     8711     +355     
- Misses        663      736      +73     
Files Coverage Δ
api/src/api/diag.ts 100.00% <100.00%> (ø)
api/src/internal/global-utils.ts 100.00% <ø> (ø)

... and 27 files with indirect coverage changes

codecov[bot] avatar Nov 11 '23 00:11 codecov[bot]

This PR 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 Mar 04 '24 06:03 github-actions[bot]

This PR 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 06 '24 06:05 github-actions[bot]

This PR 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 Jul 22 '24 06:07 github-actions[bot]