chore: remove NoopLogger from sdk and use from api #1746
Conversation
srikanthccv
requested review from
dyladan,
legendecas,
markwolff,
mayurkale22,
mwear,
naseemkullah,
obecny,
OlivierAlbertini and
vmarchaud
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1746 +/- ##
==========================================
+ Coverage 92.11% 92.15% +0.03%
==========================================
Files 166 165 -1
Lines 5571 5568 -3
Branches 1197 1197
==========================================
- Hits 5132 5131 -1
+ Misses 439 437 -2
|
|
it looks fine, @lonewolf3739 just please update to latest and run "npm run lint:fix" to fix the linting, thx |
It seems like my lint is different from ci one. I will check. |
Make sure you have up to date all node modules, sometimes you have to remove node_modules and package-lock.json from all places. |
| @@ -39,7 +35,7 @@ export class JaegerExporter implements SpanExporter { | |||
|
|
|||
| constructor(config: jaegerTypes.ExporterConfig) { | |||
| const localConfig = Object.assign({}, config); | |||
| this._logger = localConfig.logger || new NoopLogger(); | |||
| this._logger = localConfig.logger || new api.NoopLogger(); | |||
There was a problem hiding this comment.
Suggestion:
This appears to be a common pattern which is effectively necessary boilerplate code.
Provide an api function called getLogger(logger?: Logger) which returns either the provided Logger
OR a No-op implementation logger.
This allows a couple of things
- We can then don't need ANY NoopLogger class
- No more need for the boilerplate code everywhere that wants to ensure it has a logger instance
public getLogger(logger?: Logger): Logger {
if (!logger) {
logger = {
debug: () => {},
error: () => {},
warn: () => {},
info: () => {},
};
}
return logger;
}
There was a problem hiding this comment.
So this then would (potentially) become
this._logger = api.getLogger(localConfig.logger);
There was a problem hiding this comment.
I think is out of scope for this PR, please create a separate issue "discussion" and move it there. It should not be here, thx
|
Ah I was going to merge this but there are conflicts |
dyladan
added
the
Merge:LGTM
@dyladan resolved the merge conflicts :) |
|
@dyladan I resolved the conflicts again. It seems version badge service was unavailable for some time and test docs failed. I don't have chance to re-trigger pipeline. Could you please check that? |
Yeah that is a recurring problem :( |
It appears that OT removed the `NoopLogger` export from `core`: open-telemetry/opentelemetry-js#1746 This prevents app crashes when a logger is not provided.
Fixes #1738