feat(diag-logger): replace logger with diag logger #1925

MSNev · 2021-02-12T00:44:48Z

Which problem is this PR solving?

Provides solution for [API] Introduce a new global level api.diag for internal Diagnostic Logging (Part 2) #1878 and finalizes changes from [API] Introduce a new global level api.diag for internal Diagnostic Logging (Part 1) #1877 including [Environments] ConsoleLogger does not handle all console environments #1847
Completes the introduction and usage of the api.diag DiagLogger implementation.

Short description of the changes

This PR is a breaking change for all uses of the previous api.Logger, api.NoopLogger, core.LogLevel and core.ConsoleLogger

Removes the deprecated items from [API] Introduce a new global level api.diag for internal Diagnostic Logging (Part 1) #1877 including
- api.Logger interface
- api.NoopLogger class
- core.LogLevel enum
- core.ConsoleLogger
Changes the Environment.OTEL_LOG_LEVEL type from core.LogLevel to api.DiagLogLevel
Introduces common DiagLoggerConfig interface for defining the optional DiagLogger and DiagLogLevel instances all configurations that previously defined these in their own definitions.
renamed all logger properties to diagLogger

MSNev · 2021-02-12T00:50:05Z

packages/opentelemetry-api/src/diag/config.ts

+ * @param config The configuration to initialize the logger from
+ * @param getDefaultLogger - A fallback callback to fetch the specific logger if no logger is configured
+ */
+export function getDiagLoggerFromConfig(


Helper method for parsing the configuration to obtain the diagnostic logger.

Removes (most) of the need for boilerplate code to check and get the diagnostic logger.

codecov · 2021-02-12T00:51:46Z

Codecov Report

Merging #1925 (404eb7f) into main (03e741b) will increase coverage by 0.42%.
The diff coverage is 87.38%.

@@            Coverage Diff             @@
##             main    #1925      +/-   ##
==========================================
+ Coverage   92.45%   92.87%   +0.42%     
==========================================
  Files         165      179      +14     
  Lines        5472     6332     +860     
  Branches     1157     1317     +160     
==========================================
+ Hits         5059     5881     +822     
- Misses        413      451      +38

Impacted Files	Coverage Δ
...ages/opentelemetry-api-metrics/src/types/Metric.ts	`100.00% <ø> (ø)`
...ckages/opentelemetry-api/src/diag/consoleLogger.ts	`100.00% <ø> (ø)`
packages/opentelemetry-api/src/diag/logger.ts	`100.00% <ø> (ø)`
...ntelemetry-core/src/platform/BaseAbstractPlugin.ts	`60.00% <ø> (ø)`
...ackages/opentelemetry-exporter-jaeger/src/types.ts	`100.00% <ø> (ø)`
...ackages/opentelemetry-exporter-zipkin/src/types.ts	`100.00% <ø> (ø)`
...entelemetry-instrumentation/src/autoLoaderUtils.ts	`100.00% <ø> (ø)`
...es/opentelemetry-metrics/src/BaseObserverMetric.ts	`100.00% <ø> (ø)`
...ackages/opentelemetry-metrics/src/CounterMetric.ts	`100.00% <ø> (ø)`
...ackages/opentelemetry-metrics/src/MeterProvider.ts	`88.57% <ø> (-0.91%)`	⬇️
... and 74 more

packages/opentelemetry-core/src/utils/environment.ts

MSNev · 2021-02-12T00:54:10Z

packages/opentelemetry-core/test/common/logging-error-handler.test.ts

      error: errorStub,
-    });
+    } as any);


This is "safe" as the logging-error-handler only even calls error.

it's in a test anyway

MSNev · 2021-02-12T00:58:43Z

packages/opentelemetry-exporter-jaeger/src/jaeger.ts

@@ -35,7 +35,7 @@ export class JaegerExporter implements SpanExporter {

  constructor(config: jaegerTypes.ExporterConfig) {
    const localConfig = Object.assign({}, config);
-    this._logger = localConfig.logger || new api.NoopLogger();
+    this._diagLogger = getDiagLoggerFromConfig(localConfig);


This will fallback to the api.diag rather than the previous NoopLogger, so while the default api.diag is a Noop Diag Logger, if it is set this would be a change in functionality.

MSNev · 2021-02-12T00:59:48Z

packages/opentelemetry-exporter-prometheus/src/PrometheusExporter.ts

@@ -54,7 +54,7 @@ export class PrometheusExporter implements MetricExporter {
   * @param callback Callback to be called after a server was started
   */
  constructor(config: ExporterConfig = {}, callback?: () => void) {
-    this._logger = config.logger || new api.NoopLogger();
+    this._diagLogger = getDiagLoggerFromConfig(config);


This will fallback to the api.diag rather than the previous NoopLogger, so while the default api.diag is a Noop Diag Logger, if it is set this would be a change in functionality.

MSNev · 2021-02-12T01:00:35Z

packages/opentelemetry-exporter-zipkin/src/zipkin.ts

@@ -42,8 +42,8 @@ export class ZipkinExporter implements SpanExporter {

  constructor(config: zipkinTypes.ExporterConfig = {}) {
    const urlStr = config.url || ZipkinExporter.DEFAULT_URL;
-    this._logger = config.logger || new api.NoopLogger();
-    this._send = prepareSend(this._logger, urlStr, config.headers);
+    this._diagLogger = getDiagLoggerFromConfig(config);


As per other comments -- change in fallback behavior

MSNev · 2021-02-12T01:02:08Z

packages/opentelemetry-instrumentation/src/autoLoader.ts

  const meterProvider = options.meterProvider || metrics.getMeterProvider();
-  const logger =
-    options.logger || tracerWithLogger?.logger || new api.NoopLogger();
+  const diagLogger = getDiagLoggerFromConfig(options);


As per other comment -- change in fallback behavior.

MSNev · 2021-02-12T01:04:53Z

packages/opentelemetry-metrics/src/Meter.ts

@@ -55,7 +59,10 @@ export class Meter implements api.Meter {
    config: MeterConfig = {}
  ) {
    const mergedConfig = merge({}, DEFAULT_CONFIG, config);
-    this._logger = mergedConfig.logger || new ConsoleLogger(config.logLevel);
+    this._diagLogger = getDiagLoggerFromConfig(
+      mergedConfig,


Unlike other usages of getDiagLoggerFromConfig(), the fallback logic here will still result in a console logger. And the logging level will still use the configured config.diagLogLevel value as this occurs internally as part of the helper.

MSNev · 2021-02-12T01:05:32Z

packages/opentelemetry-metrics/src/MeterProvider.ts

-    this.logger =
-      mergedConfig.logger ?? new ConsoleLogger(mergedConfig.logLevel);
+    this.diagLogger = getDiagLoggerFromConfig(
+      mergedConfig,


Same as previous comment -- no change in fallback behavior or logging level.

packages/opentelemetry-core/src/common/logging-error-handler.ts

packages/opentelemetry-core/src/utils/environment.ts

dyladan · 2021-02-12T14:07:41Z

packages/opentelemetry-core/test/common/logging-error-handler.test.ts

      error: errorStub,
-    });
+    } as any);


it's in a test anyway

dyladan · 2021-02-12T14:09:02Z

packages/opentelemetry-core/test/platform/browser/BasePlugin.test.ts

@@ -17,24 +17,24 @@
 import {
  NOOP_TRACER,
  NoopTracerProvider,
-  NoopLogger,
+  createNoopDiagLogger,


Is this exported for any reason other than testing? If not, I would prefer it not be exported from the API package as it then becomes part of the API we have to support.

It is mostly for testing (as that is the most usage), to remove would require that all test usages would need to be refactored to not use it (which in most cases they shouldn't need at all). Do we want to do that as part of this PR?

It does raise the following questions

Do we or should we have some sort of test "helpers" as for this a MockLogger that records all usages in memory would be useful?

For all existing usages of new NoopLogger() what do we tell them for upgrading, as there may be at least one user that really want to sink all messages?

On the above, where should I add documentation on usage / upgrade notes for the release notes, so that we get people upgrading and using this in the prescribed way?

Searching across all usages the only non-api and non-test usage is in opentelemetry-shim-opentracing\src\shim.ts which is currently
this._diagLogger = diagLogger || api.createNoopDiagLogger();

We could change this to following, but this would also be a function change -- does anyone know if this would be ok? As it would affect anyone using OpenTracing Tracers (as per the Readme)
this._diagLogger = diagLogger || diag;

hmm i'm not sure how that would affect the shim as i'm not that familiar with opentracing.

Looks like either should be safe as this logger is only used in the shim and not passed to opentracing itself.

Do we or should we have some sort of test "helpers" as for this a MockLogger that records all usages in memory would be useful?

I think something like that can be added later. For now I don't think it's something we need to focus on

For all existing usages of new NoopLogger() what do we tell them for upgrading, as there may be at least one user that really want to sink all messages?

if an end user is passing a NoopLogger into a component, they can just pass a disabled DiagLogger into it. Unless i've missed something?

On the above, where should I add documentation on usage / upgrade notes for the release notes, so that we get people upgrading and using this in the prescribed way?

in the README.md there is an upgrade guide. You can go ahead and add a new entry.

Looks like either should be safe as this logger is only used in the shim and not passed to opentracing itself.

I've left this alone for now.

if an end user is passing a NoopLogger into a component, they can just pass a disabled DiagLogger into it. Unless i've missed something?

Correct, you've not missed anything

in the README.md there is an upgrade guide. You can go ahead and add a new entry.

I've added a new 0.17 -> section to the readme.

packages/opentelemetry-exporter-collector-grpc/test/CollectorMetricExporter.test.ts

packages/opentelemetry-api/src/diag/config.ts

obecny

I think this went into completely different direction than what we have discussed and agreed for.
The main idea was to be able to do something like this anywhere and always without worrying about whether the logger was set or not, and also to avoid parsing a logger everywhere.

api.logger.debug('....
api.logger.info('...
// etc. etc.

But now I see things like

diagLogger && diagLogger.debug('xhr success', body);

This solution was suppose to make it lighter smaller and easier. But what we have now is a lot of new unnecessary duplicated code which completely changed the initial idea and for me it is not how it should work.
Also the migration information in README.md just shows the complexity of this solution too. It is just over engineered.

MSNev · 2021-02-16T03:54:40Z

Hi Bart,

I completely disagree with your assessment.

I think this went into completely different direction than what we have discussed and agreed for.
The main idea was to be able to do something like this anywhere and always without worrying about whether the logger was set or not, and also to avoid parsing a logger everywhere.

This is exactly what we have available.

api.diag.debug('...');
api.diag.info('...');

The additional code (specifically thegetDiagLoggerFromConfig is to support the existing functionality without completely refactoring and changing (possibly breaking) the existing usages, especially as several components (including AWS and Collector see CollectorExporterConfigBase) currently have declared in their config the ability to pass a Logger to use. So this helper moves all of the usages into a single location, one place to change, fix and possible find and remove any usages in a follow up PR.

And at the end of the day this falls back to api.diag when no logger is defined or passed.

Which brings me to the OptionalDiagLogger type, this is so any current usage of this can be replaced with either "null" or "undefined" (null is better from a minification perspective) as it will (in most cases) fallback to api.diag. The most cases are called out in the next response.

And yes, there are also several additional logging functions / levels, and these will become more useful as we unify the code to only use the single api.diag instance.

But now I see things like
diagLogger && diagLogger.debug('xhr success', body);

These 2 instances are because the current implementations pass the diagLogger as the 3rd argument and before any optional arguments. For this there are 2 basic options.

Make it optional and only log if it's passed in, primarily to avoid causing further refactoring as this PR is already TOO big.
- Based on the current usages (in this repo) the diagLogger is always passed in from a getDiagLoggerFromConfig() call and as such the additional prefix check of diagLogger && is just being defensive for any other consumers.
- This could of also have been rewritten as (diagLogger || api.diag).debug(), but this could change the logging behavior.
Remove the parameter completely and just use api.diag, this has potentially un-intended side effects
- Removal would cause additional pain to anyone upgrading (and possible make this PR even larger)
- More importantly, it would change the logging behavior as the current collector base has the ability to define a logger instance to use.

This solution was suppose to make it lighter smaller and easier. But what we have now is a lot of new unnecessary duplicated code which completely changed the initial idea and for me it is not how it should work.

100% disagree with this statement, this is removing duplicated code from across the repo... Where is it duplicated?

Basic usage is now api.diag or getDiagLoggerFromConfig

And by defining the DiagLoggerConfig interface it also removes the definitions from all configs (so we now have a single location for config based diagnostic logging) rather than all over the code.
AND all code that fetches a logger from config now use this.diagLogger = getDiagLoggerFromConfig(config); and this falls back to api.diag with possible config based Log Level filtering (The only exceptions (called out below) are to retain backward compatibility when using a Console logger when nothing is provided)

Also the migration information in README.md just shows the complexity of this solution too.

The readme is covering "How to upgrade", I defined the "New usage patterns" first to highlight that this is how things should be doing (moving forward) and as part of this, it is also covering some of the edge cases that are currently occurring in the SDK.
- I mention more about the getDefaultLogger callback below, which is the edge cases. We can remove them from the readme and leave any "upgrader" to troll the code or raise an issue.
- Specifically lines 312-325 from the current iteration of the README
And the first paragraph of the Direct update path for existing code states
- Without refactoring your existing code to use api.diag or getDiagLoggerFromConfig (recommended path), below is how you would directly upgrade to the newer version so you can get compiling and running.
- We can make this wording more forceful if you like -- Or simply remove and deal with the Issues that will be raised.
- But ideally, we want people to upgrade and not just stay on the previous versions and raise issues.
- If the readme is the wrong place for this 2nd set of details, then what would be the alternate location?

As for the extra helpers defined in this section

createNoopDiagLogger() this is a direct replacement of all usages of new NoopLogger() (see previous discussion in this PR on this), it's primarily for testing (without refactoring EVERY test usage) except for a single usage in the OpenTracer Shim.
core.LogLevel -> api.DiagLogLevel is a direct replacement
core.ConsoleLogger -> api.DiagConsoleLogger is a direct replacement
createLogLevelDiagLogger() as discussed in the previous PR this is a wrapper 9sink) which is not only used internally but is "available" for external usage so they can quickly upgrade and additionally, no-one needs to duplicate this code in every DiagLogger instance they create, it also means we can add/update the log level handling in a single location.

It is just over engineered.

This is the only portion of your response where I partially agree and it's only in relation to the getDefaultLogger callback function of getDiagLoggerFromConfig(), which I would have preferred not to implement, but I did for reasons I believe I've already mentioned to retain current backward compatability without duplicating this code.

So you have requested changes, but what exactly are the changes you are suggesting?

Full Breaking changes with only able to use api.diag?
- And therefore removing the ability to pass a logger by config?
- Stop passing as a parameter? (I'm ok with this one except for the above issues (PR size and functional change))
Don't export createLogLevelDiagLogger()?
- I'm ok with this too. as long as we sign out for the additional breaking changes and we tell people that if they want to use their own logger they either set it as the api.diag.setLogger() or via config so it's automatically wrapper via getDiagLoggerFromConfig
- Additional breaks that this would incur in the repo
  - opentelemetry-core/src/common/logging-error-handler.ts
  - opentelemetry-metrics/src/Meter.ts
  - opentelemetry-metrics/src/MeterProvider.ts
  - opentelemetry-node/src/NodeTracerProvider.ts
Remove the callback getDefaultLogger function from getDiagLoggerFromConfig and all of the additional breaking issues this will incur?

obecny · 2021-02-16T12:33:28Z

@MSNev

Don't rename all logger internals into diagLogger. DiagLogger for me is just a logger which can be replaced by any other logger and all components should be still logging easily. All sigs are using logger, in many different libraries you have logger it is straightforward, intuitive and easy to understand, we are adding here extra complexity which is not "standard". But instead I would remove it completely - explaining why in the next point.
Extra helpers to "make things easier" they might just produce more confuses than answers, do you really have to use it, what if you don't, are you still able to have logger without it, what does this functions really do ? Without reading a code you might not really find those answers.

If you look at Instrumentation class you will see that things like tracerProvider and meterProvider can be set and changed at any time. If you don't do it they will be taken from global api. This way it doesn't matter if you set it or not you can always call it without checking for existence and nothing will be broken, you can call api global to change provider and it will be transparent and at the same time you can change a provider for just one instrumentation. The same approach we should have for logger. This can be achieved in 2 ways, class Instrumentation can have setLogger function which will fully satisfy cases when you want to change logger for only one "component" or make a new class which can be extended.
We can discuss If this is really needed now or it can be done later. Personally I will be fine with not adding this option yet. As a comparison we have a globalErrorHandler which works fine and we didn't have to create yet functionality which will allow us to set a different error handler for certain component only.

If I were to implement this later or now I would still make it differently than having some api helpers functions for parsing config etc.. I would create one class some BaseClassWithLoggerToBeExtended which will have a logger , setLogger. Then all you have to do with any class is just extend from this class. For example

class BaseClassWithLoggerToBeExtended {
  logger: api.Logger;
  constructor() {
    this.logger = api.logger;
  }
  setLogger(logger) {
    this.logger = logger;
  }
}

// And then

export abstract class InstrumentationAbstract<T = any> extends BaseClassWithLoggerToBeExtended
  implements types.Instrumentation {
  protected _config: types.InstrumentationConfig;

  private _tracer: api.Tracer;
  private _meter: Meter;

  constructor(
    public readonly instrumentationName: string,
    public readonly instrumentationVersion: string,
    config: types.InstrumentationConfig = {}
  ) {
    super();
    // ........

This way if you want to allow setting a different logger you can do it easily just extend class and then instead of api.logger.log you will be able to do this

  this.logger.info('........ // no need for parsing, adding extra logic in many places in whole codebase

  // change the logger now only for this component
  someInstrumentation.setLogger(myOwnLogger);

  // revert back to global
  someInstrumentation.setLogger(api.logger);

  // or 
  api.setLogger

But once more we can extend anything (exporters, instrumentation, providers etc.) with BaseClassWithLoggerToBeExtended (figure out better name) only if we really want to be able to set a different logger for certain component this doesn't need to be done now. We will not break things later. We just have to remove now the logger option that we have in so many places.

It was brought up previously and I want to brought this up again - too many levels of logging, I know that you had an argument that we might use it in a future but I think we mind end up with having functionality that is used in 0,1% cases. We should add things if we really need it, not opposite, otherwise we are creating unnecessary things which will be harder to remove and they complicate things in longer run. Adding new stuff is always easier, then removing.

Summarizing in a short

Get rid of helpers
Remove logger option from many places
Reduce number of levels to what we had and what is supported by default
Discuss and decide if we want to add BaseClassWithLoggerToBeExtended approach - if we have this we can update classes that we think should be able to use individual logger. But this can be done at anytime without breaking anything

And lastly after these changes the upgrade guide in readme will become this (without allowing yet to set individual logger)
// previously
const myClass = new Class({logger: new ConsoleLogger()}
// now
api.setLogger(new Diag.....)
// or even
api.setLogger(console);

….NoopLogger and core.ConsoleLogger

MSNev · 2021-02-17T04:47:38Z

packages/opentelemetry-plugin-grpc-js/src/grpcJs.ts

-   * Public reference to the protected BasePlugin `_logger` instance to be used by this
-   * plugin's external helper functions
-   */
-  get logger(): Logger {


Turns out this was the only getter and after refactoring to only use api.diag, this appears to be a safe removal.

I think the getter we were talking about was changing the plugin/instrumentation base logger properties to be getters that pointed to diag to have a point to add a namespace easily in the future.

dyladan

This looks like very nearly exactly what we talked about yesterday thanks for making the changes. Aside from the two getter comments I made I'm happy with this and will leave my green check in place :)

dyladan · 2021-02-17T14:16:20Z

packages/opentelemetry-core/src/platform/BaseAbstractPlugin.ts

@@ -27,7 +27,6 @@ export abstract class BaseAbstractPlugin<T> implements Plugin<T> {
  protected _config!: PluginConfig;
  protected _internalFilesExports!: { [module: string]: unknown }; // output for internalFilesExports
  protected readonly _internalFilesList?: PluginInternalFiles; // required for internalFilesExports
-  protected _logger!: Logger;


I thought this was going to point to api.diag so we could prevent breaking existing plugins

dyladan · 2021-02-17T14:20:32Z

packages/opentelemetry-instrumentation/src/instrumentation.ts

@@ -40,12 +39,8 @@
      enabled: true,
      ...config,
    };
-    this._logger = this._config.logger || new api.NoopLogger();


Same here. I thought we were going to have a getter to api.diag here so that we would have a place to easily add a namespace in the future. For example in the future it could be populated with some logger created by api.diag.withNamespace(instrumentationName)

vmarchaud · 2021-02-17T15:44:16Z

packages/opentelemetry-core/src/common/logging-error-handler.ts

  return (ex: Exception) => {
-    logger!.error(stringifyException(ex));


i think we could remove the loggingErrorHandler so we only the use the diag logger in a future PR

No I think we should keep it. The global error handler serves a different (though related) function and having an option to easily push those errors to logs is probably what most people will want most of the time.

Well its purpose was for internal components (exporters etc) so i'm not sure why we could not use the diag logger instead ?

This gives users the option to trigger some behavior on an unexpected instrumentation failure like restarting the service or preventing startup or something.

vmarchaud

i prefer this design, it match how other API namespace works so 👍

obecny

The changes looks great, they are just few minor things that left including readme upgrade guidelines

obecny · 2021-02-17T14:43:47Z

benchmark/tracer.js

@@ -4,7 +4,8 @@ const benchmark = require('./benchmark');
 const opentelemetry = require('../packages/opentelemetry-api');
 const { BasicTracerProvider, BatchSpanProcessor, InMemorySpanExporter, SimpleSpanProcessor } = require('../packages/opentelemetry-tracing');

-const diagLogger = opentelemetry.createNoopDiagLogger();
+// Clear any previous global logger
+opentelemetry.diag.setLogger(null);


I guess opentelemetry.diag.setLogger() should also set noop logger ?

Calling setLogger() with a "falsey" value will default to a noop implementation

obecny · 2021-02-17T14:49:54Z

packages/opentelemetry-api/test/diag/logLevel.test.ts

-            }
-          });
-        });
-
        levelMap.forEach(masterLevelMap => {
          it(`diag setLogLevel is not ignored when set to ${masterLevelMap.message} and using default logger to log ${fName} message with ${map.message} level`, () => {


Nit: split it into more levels, when using word "when" - then just add "describe"

describe('when logger is set to ${masterLevelMap.message}', ()=> { it('should .....' describe('AND using default logger ..... it('should ......... })

This is not a blocker, it is nice to have for better readability and testing

obecny · 2021-02-17T16:13:52Z

getting-started/traced-example/tracing.js

 const { NodeTracerProvider } = require("@opentelemetry/node");
 const { SimpleSpanProcessor } = require("@opentelemetry/tracing");
 const { ZipkinExporter } = require("@opentelemetry/exporter-zipkin");
 const { registerInstrumentations } = require('@opentelemetry/instrumentation');

-const provider = new NodeTracerProvider({ logLevel: LogLevel.ERROR });
+const provider = new NodeTracerProvider({ diagLogLevel: DiagLogLevel.ERROR });


I think this should be removed

Suggested change

const provider = new NodeTracerProvider({ diagLogLevel: DiagLogLevel.ERROR });

const provider = new NodeTracerProvider();

Yes, it should

obecny · 2021-02-17T16:14:30Z

getting-started/ts-example/traced-example/tracing.ts

 const provider: NodeTracerProvider = new NodeTracerProvider({
-  logLevel: LogLevel.ERROR,
+  diagLogLevel: DiagLogLevel.ERROR,


here the same the diagLogLevel should be removed

obecny · 2021-02-17T16:15:21Z

packages/opentelemetry-tracing/src/config.ts

@@ -23,7 +23,7 @@ import { AlwaysOnSampler, getEnv } from '@opentelemetry/core';
 * used to extend the default value.
 */
 export const DEFAULT_CONFIG = {
-  logLevel: getEnv().OTEL_LOG_LEVEL,
+  diagLogLevel: getEnv().OTEL_LOG_LEVEL,


should this be removed ?

obecny · 2021-02-17T16:15:42Z

packages/opentelemetry-metrics/src/types.ts

@@ -44,7 +37,7 @@ export interface MeterConfig {

 /** Default Meter configuration. */
 export const DEFAULT_CONFIG = {
-  logLevel: getEnv().OTEL_LOG_LEVEL,
+  diagLogLevel: getEnv().OTEL_LOG_LEVEL,


should this be removed ?

Nice catches, I obviously missed diagLogLevel removals -- will update (remove) all.

obecny · 2021-02-17T16:31:24Z

README.md

@@ -244,6 +244,156 @@ To request automatic tracing support for a module not on this list, please [file

 ## Upgrade guidelines

+### 0.17.0 to ???
+
+[PR-1880](https://github.com/open-telemetry/opentelemetry-js/pull/1880) feat(diag-logger): introduce a new global level api.diag for internal diagnostic logging


I thought we agreed that this whole readme section will become also much smaller and simpler.
From a user perspective we have to mention the basic scenarios, So I would keep this only:

Defining logger via config option has been removed in favor of global diag logger.

Setting logger

import { DiagLogLevel, DiagConsoleLogger, diag } from "@opentelemetry/api"; diag.setLogger(new DiagConsoleLogger()); diag.setLogLevel(DiagLogLevel.ERROR);

Using logger anywhere in a code

import { diag } from "@opentelemetry/api"; diag.debug("...");

Setting logger back to noop

import { diag } from "@opentelemetry/api"; diag.setLogger();

I contemplated that -- which is why I also added the recommended approach to each.
Will remove the "get compiling" steps as it's mostly obvious now I think.

obecny

lgtm, thx for changes

open-telemetry#1925)

MSNev requested review from dyladan, Flarna, johnbley, legendecas, markwolff, mayurkale22, mwear, naseemkullah, obecny, OlivierAlbertini and vmarchaud as code owners February 12, 2021 00:44

MSNev commented Feb 12, 2021

View reviewed changes

packages/opentelemetry-core/src/utils/environment.ts Outdated Show resolved Hide resolved

MSNev commented Feb 12, 2021

View reviewed changes

dyladan reviewed Feb 12, 2021

View reviewed changes

dyladan approved these changes Feb 12, 2021

View reviewed changes

MSNev commented Feb 13, 2021

View reviewed changes

packages/opentelemetry-api/src/diag/config.ts Outdated Show resolved Hide resolved

vmarchaud approved these changes Feb 14, 2021

View reviewed changes

obecny suggested changes Feb 16, 2021

View reviewed changes

MSNev added 2 commits February 16, 2021 20:34

feat(diag-logger): part 2 - breaking changes - remove api.Logger, api…

afbf986

….NoopLogger and core.ConsoleLogger

fix: address review comments, add upgrade notes and tests

88db182

fix: additional breaking changes and removal of config initialization

f191b10

MSNev force-pushed the MSNev/AddDiagLogger branch from 76413f0 to f191b10 Compare February 17, 2021 04:45

MSNev commented Feb 17, 2021

View reviewed changes

fix: browser test failures because of default logging level

e9ee627

dyladan reviewed Feb 17, 2021

View reviewed changes

vmarchaud reviewed Feb 17, 2021

View reviewed changes

vmarchaud approved these changes Feb 17, 2021

View reviewed changes

obecny reviewed Feb 17, 2021

View reviewed changes

fix: removed missed DiagLogLevel and make diag.setLogger() arg optional

37226fe

dyladan added the breaking label Feb 17, 2021

obecny approved these changes Feb 17, 2021

View reviewed changes

dyladan merged commit 62f8695 into open-telemetry:main Feb 17, 2021

dyladan changed the title ~~feat(diag-logger): part 2 - breaking changes - remove api.Logger, api…~~ feat(diag-logger): implement diag logger Feb 17, 2021

dyladan changed the title ~~feat(diag-logger): implement diag logger~~ feat(diag-logger): replace logger with diag logger in most places Feb 17, 2021

dyladan changed the title ~~feat(diag-logger): replace logger with diag logger in most places~~ feat(diag-logger): replace logger with diag logger Feb 17, 2021

Flarna mentioned this pull request Feb 18, 2021

chore: release 0.17.0 proposal #1926

Merged

MSNev deleted the MSNev/AddDiagLogger branch January 10, 2022 19:57

dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022

feat(diag-logger): part 2 - breaking changes - remove api.Logger, api… (

1dda916

open-telemetry#1925)

dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022

feat(diag-logger): part 2 - breaking changes - remove api.Logger, api… (

62f19b0

open-telemetry#1925)

		return (ex: Exception) => {
		logger!.error(stringifyException(ex));

	const provider = new NodeTracerProvider({ diagLogLevel: DiagLogLevel.ERROR });
	const provider = new NodeTracerProvider();

feat(diag-logger): replace logger with diag logger #1925

feat(diag-logger): replace logger with diag logger #1925

Conversation

MSNev commented Feb 12, 2021 • edited Loading

Which problem is this PR solving?

Short description of the changes

Choose a reason for hiding this comment

codecov bot commented Feb 12, 2021 • edited Loading

Codecov Report

MSNev Feb 12, 2021 • edited Loading

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

MSNev Feb 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSNev Feb 12, 2021 • edited Loading

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

MSNev Feb 12, 2021 • edited Loading

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

obecny left a comment • edited Loading

Choose a reason for hiding this comment

MSNev commented Feb 16, 2021

obecny commented Feb 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmarchaud left a comment

Choose a reason for hiding this comment

obecny left a 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

obecny Feb 17, 2021 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obecny left a comment

Choose a reason for hiding this comment

MSNev commented Feb 12, 2021 •

edited

Loading

codecov bot commented Feb 12, 2021 •

edited

Loading

MSNev Feb 12, 2021 •

edited

Loading

MSNev Feb 12, 2021 •

edited

Loading

MSNev Feb 12, 2021 •

edited

Loading

MSNev Feb 12, 2021 •

edited

Loading

obecny left a comment •

edited

Loading

obecny commented Feb 16, 2021 •

edited

Loading

obecny Feb 17, 2021 •

edited

Loading