feat(api): Optionally suppress warning about logger being overwritten #3366

SimenB · 2022-10-31T08:58:13Z

Which problem is this PR solving?

We use pino as logger, meaning we want it instrumented. And once it is instrumented, we'd like to use it as the logger for open telemetry. But up until instrumentation is done, we'd like to have a logger active.

diag.setLogger(new DiagConsoleLogger(), DiagLogLevel.INFO);

// create provider

registerInstrumentations({
  tracerProvider: provider,
  instrumentations: [new PinoInstrumentation()],
});

provider.register();

const pino = require('pino');

const logger = pino();

diag.setLogger(
  {
    error: (msg, ...args) => logger.error(util.format(msg, args)),
    // etc.
  },
  DiagLogLevel.INFO,
);

However, this triggers the override warning. This PR adds an optional flag to suppress this warning when the caller knows it'll override one.

Fixes # N/A

Short description of the changes

This PR adds an optional flag to suppress a warning about overwritten logger when the caller knows it'll override one.

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Made the change in my node_modules and verified warning is no longer printed

Checklist:

Followed the style guidelines of this project
Unit tests have been added
Documentation has been updated

vmarchaud · 2022-10-31T09:07:36Z

Maybe we could have an options object instead of a single param ? That would avoid breaking changes later on

SimenB · 2022-10-31T09:26:43Z

Sure 👍 log level should probably be merged with that in a future major?

vmarchaud · 2022-10-31T09:44:00Z

Sure +1 log level should probably be merged with that in a future major?

I admit i'm not sure if its something that we want actually, we'll see other opinion on this

codecov · 2022-10-31T09:56:52Z

Codecov Report

Merging #3366 (c96858a) into main (c6ff50e) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head c96858a differs from pull request most recent head 455e948. Consider uploading reports for the commit 455e948 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3366      +/-   ##
==========================================
- Coverage   93.25%   93.24%   -0.02%     
==========================================
  Files         247      247              
  Lines        7341     7344       +3     
  Branches     1509     1511       +2     
==========================================
+ Hits         6846     6848       +2     
- Misses        495      496       +1

Impacted Files	Coverage Δ
api/src/diag/types.ts	`100.00% <ø> (ø)`
api/src/api/diag.ts	`100.00% <100.00%> (ø)`
...-trace-base/src/platform/node/RandomIdGenerator.ts	`87.50% <0.00%> (-6.25%)`	⬇️

SimenB · 2022-10-31T09:59:05Z

Where should I add a changelog entry? https://github.com/open-telemetry/opentelemetry-js/blob/main/api/CHANGELOG.md seems autogenerated

dyladan · 2022-11-01T14:12:54Z

Where should I add a changelog entry? main/api/CHANGELOG.md seems autogenerated

It was, but now we have to manually add entries. Add an ## Unreleased section at the top for these changes.

dyladan · 2022-11-01T14:14:26Z

api/src/api/diag.ts

+      logLevel: DiagLogLevel = DiagLogLevel.INFO,
+      options: LoggerOptions = {}


You also need to update the types https://github.com/open-telemetry/opentelemetry-js/pull/3366/files#diff-27d843b1252efc09aad875a53b4b2bb9a4b98180667b1669d46067af08662bedR125

Wonder if we want to add a third parameter or make the second parameter an overload?

updated the type

happy to make it an overload if you want? 🙂

We are avoiding overloading API methods that need to be implemented in SDK libraries. However, this method is not part of an interface that needs to be implemented in SDKs.

So I believe it would be intuitive to be overloaded as an option bag.

the self.logger use is done there intentionally to aid with code minification. what help do you need?

if you revert the last commit, you can see the typescript errors

if you revert the last commit, you can see the typescript errors

The compilation errors can be suppressed if the two method declarations are merged into a single one, e.g.

/** * Set the global DiagLogger and DiagLogLevel. * If a global diag logger is already set, this will override it. * * @param logger - [Optional] The DiagLogger instance to set as the default logger. * @param logLevelOrOptions - [Optional] And object which can contain `logLevel: DiagLogLevel` used to filter logs sent to the logger. If not provided it will default to INFO. The object may also contain `suppressOverrideMessage: boolean` which will suppress the warning normally logged when a logger is already registered. * @returns true if the logger was successfully registered, else false */ public setLogger!: (logger: DiagLogger, logLevelOrOptions?: DiagLogLevel | LoggerOptions) => boolean;

As for why those methods need to be defined in this non-conventional way, I traced back to #1880 and didn't find an explanation. Maybe @MSNev can chime in and share more about it?

Ok, I've blocked this PR as we should NOT convert the property into a function as this becomes less compressible through minification.

And then will cause me to re-implement this original change again in the sandbox to have it contributed back here.

This pattern has major implications on the overall browser bundle sizes (depending on the module format and ES level being targetted). And this is one of the techniques that I WILL be investigating the viability and performing across the entire JS and Contrib codebases in the sandbox (when I get it going -- HOPEFULLY shortly)

SimenB · 2022-11-02T08:47:18Z

changelog entry added

naseemkullah · 2022-11-03T23:11:41Z

Could we not just lower the log level to debug/trace for that message? Is it easy to accidentally overwrite the logger?

api/src/api/diag.ts

This reverts commit 50fed69.

legendecas · 2022-11-07T16:32:42Z

@naseemkullah Could we not just lower the log level to debug/trace for that message? Is it easy to accidentally overwrite the logger?

I'd find the example shown in the OP is persuading to suppress the message entirely: the override is intentional so there is no need to log anymore.

legendecas

LGTM. Thank you for refactoring the type interfaces!

legendecas · 2022-11-08T15:39:37Z

hi @dyladan, as @SimenB requested at #3388 (comment), do you think it is possible for us to include this PR in #3340? Or should we go ahead as the release has been proposed for weeks?

dyladan · 2022-11-08T15:49:39Z

Let's include it

dyladan · 2022-11-08T16:15:11Z

I updated some of the typedoc comments.

api/src/diag/types.ts

SimenB

I think?

api/src/diag/types.ts

SimenB · 2022-11-08T20:12:40Z

🎉

naseemkullah · 2022-11-09T13:42:33Z

@naseemkullah Could we not just lower the log level to debug/trace for that message? Is it easy to accidentally overwrite the logger?

I'd find the example shown in the OP is persuading to suppress the message entirely: the override is intentional so there is no need to log anymore.

Intentional things can be debug logged though, just wondering if there is any reason it was logged as warn

feat(api): Optionally suppress warning about logger being overwritten

566e3ec

SimenB requested a review from a team as a code owner October 31, 2022 08:58

options bag

170458a

vmarchaud approved these changes Oct 31, 2022

View reviewed changes

vmarchaud requested a review from a team October 31, 2022 09:44

dyladan reviewed Nov 1, 2022

View reviewed changes

SimenB added 2 commits November 2, 2022 09:45

update types

c2b8ae2

changelog

f1d3529

SimenB added 4 commits November 2, 2022 09:47

changelog heading

dc094f0

document in api

f3c52a5

overload

ea86043

move impl

50fed69

SimenB force-pushed the optionally-suppress-diag-override-warning branch from 709e1d1 to 50fed69 Compare November 2, 2022 10:59

MSNev reviewed Nov 4, 2022

View reviewed changes

api/src/api/diag.ts Outdated Show resolved Hide resolved

SimenB added 5 commits November 4, 2022 18:59

Revert "move impl"

0c41666

This reverts commit 50fed69.

Merge branch 'main' into optionally-suppress-diag-override-warning

aa36510

no overload

bb6df0a

lint

2e37304

use interface to provide overload

4a4628f

SimenB mentioned this pull request Nov 7, 2022

Release proposal 1.8.0 / 0.34.0 #3388

Closed

legendecas approved these changes Nov 7, 2022

View reviewed changes

Merge branch 'main' into optionally-suppress-diag-override-warning

598bc1e

Update typedoc comments

22fe3c4

SimenB commented Nov 8, 2022

View reviewed changes

api/src/diag/types.ts Outdated Show resolved Hide resolved

Update api/src/diag/types.ts

56500bd

SimenB commented Nov 8, 2022

View reviewed changes

api/src/diag/types.ts Outdated Show resolved Hide resolved

api/src/diag/types.ts Outdated Show resolved Hide resolved

dyladan added 2 commits November 8, 2022 13:43

Merge branch 'main' into optionally-suppress-diag-override-warning

f6bec3a

Remove optional text

455e948

dyladan merged commit baf0fee into open-telemetry:main Nov 8, 2022

SimenB deleted the optionally-suppress-diag-override-warning branch November 8, 2022 20:12

naseemkullah mentioned this pull request Mar 3, 2023

diag/setLogger has confusing warning which seems like an Error when logger gets overwritten #3648

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): Optionally suppress warning about logger being overwritten #3366

feat(api): Optionally suppress warning about logger being overwritten #3366

SimenB commented Oct 31, 2022

vmarchaud commented Oct 31, 2022

SimenB commented Oct 31, 2022

vmarchaud commented Oct 31, 2022

codecov bot commented Oct 31, 2022 •

edited

SimenB commented Oct 31, 2022

dyladan commented Nov 1, 2022

dyladan Nov 1, 2022

dyladan Nov 1, 2022

SimenB Nov 2, 2022

SimenB Nov 2, 2022

legendecas Nov 2, 2022

dyladan Nov 2, 2022 •

edited

SimenB Nov 2, 2022

legendecas Nov 4, 2022

MSNev Nov 4, 2022

MSNev Nov 4, 2022

SimenB commented Nov 2, 2022

naseemkullah commented Nov 3, 2022

legendecas commented Nov 7, 2022

legendecas left a comment

legendecas commented Nov 8, 2022

dyladan commented Nov 8, 2022

dyladan commented Nov 8, 2022

SimenB left a comment

SimenB commented Nov 8, 2022

naseemkullah commented Nov 9, 2022

		logLevel: DiagLogLevel = DiagLogLevel.INFO,
		options: LoggerOptions = {}

feat(api): Optionally suppress warning about logger being overwritten #3366

feat(api): Optionally suppress warning about logger being overwritten #3366

Conversation

SimenB commented Oct 31, 2022

Which problem is this PR solving?

Short description of the changes

Type of change

How Has This Been Tested?

Checklist:

vmarchaud commented Oct 31, 2022

SimenB commented Oct 31, 2022

vmarchaud commented Oct 31, 2022

codecov bot commented Oct 31, 2022 • edited

Codecov Report

SimenB commented Oct 31, 2022

dyladan commented Nov 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan Nov 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB commented Nov 2, 2022

naseemkullah commented Nov 3, 2022

legendecas commented Nov 7, 2022

legendecas left a comment

Choose a reason for hiding this comment

legendecas commented Nov 8, 2022

dyladan commented Nov 8, 2022

dyladan commented Nov 8, 2022

SimenB left a comment

Choose a reason for hiding this comment

SimenB commented Nov 8, 2022

naseemkullah commented Nov 9, 2022

codecov bot commented Oct 31, 2022 •

edited

dyladan Nov 2, 2022 •

edited