Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 18 commits into from
Nov 8, 2022
Merged

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

Changes from 2 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
566e3ec
feat(api): Optionally suppress warning about logger being overwritten
SimenB Oct 31, 2022
170458a
options bag
SimenB Oct 31, 2022
c2b8ae2
update types
SimenB Nov 2, 2022
f1d3529
changelog
SimenB Nov 2, 2022
dc094f0
changelog heading
SimenB Nov 2, 2022
f3c52a5
document in api
SimenB Nov 2, 2022
ea86043
overload
SimenB Nov 2, 2022
50fed69
move impl
SimenB Nov 2, 2022
0c41666
Revert "move impl"
SimenB Nov 4, 2022
aa36510
Merge branch 'main' into optionally-suppress-diag-override-warning
SimenB Nov 4, 2022
bb6df0a
no overload
SimenB Nov 4, 2022
2e37304
lint
SimenB Nov 7, 2022
4a4628f
use interface to provide overload
SimenB Nov 7, 2022
598bc1e
Merge branch 'main' into optionally-suppress-diag-override-warning
dyladan Nov 8, 2022
22fe3c4
Update typedoc comments
dyladan Nov 8, 2022
56500bd
Update api/src/diag/types.ts
SimenB Nov 8, 2022
f6bec3a
Merge branch 'main' into optionally-suppress-diag-override-warning
dyladan Nov 8, 2022
455e948
Remove optional text
dyladan Nov 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 7 additions & 2 deletions api/src/api/diag.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ import {

const API_NAME = 'diag';

interface LoggerOptions {
suppressOverrideMessage?: boolean;
}

/**
* Singleton object which represents the entry point to the OpenTelemetry internal
* diagnostic API
Expand Down Expand Up @@ -67,7 +71,8 @@ export class DiagAPI implements DiagLogger {

self.setLogger = (
logger: DiagLogger,
logLevel: DiagLogLevel = DiagLogLevel.INFO
logLevel: DiagLogLevel = DiagLogLevel.INFO,
options: LoggerOptions = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

happy to make it an overload if you want? 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@dyladan dyladan Nov 2, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

) => {
if (logger === self) {
// There isn't much we can do here.
Expand All @@ -83,7 +88,7 @@ export class DiagAPI implements DiagLogger {
const oldLogger = getGlobal('diag');
const newLogger = createLogLevelDiagLogger(logLevel, logger);
// There already is an logger registered. We'll let it know before overwriting it.
if (oldLogger) {
if (oldLogger && !options.suppressOverrideMessage) {
const stack = new Error().stack ?? '<failed to generate stacktrace>';
oldLogger.warn(`Current logger will be overwritten from ${stack}`);
newLogger.warn(
Expand Down