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(diag-logger): introduce a new global level api.diag for internal diagnostic logging #1880

Merged
merged 6 commits into from
Feb 10, 2021
Merged

feat(diag-logger): introduce a new global level api.diag for internal diagnostic logging #1880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
692e582
feat(diag-logger): introduce a new global level api.diag for internal…
MSNev Jan 27, 2021
f26614e
fix(test): ci environment doesn't handle replacing console with a mock
MSNev Jan 28, 2021
7c834ef
fix: address review comments
MSNev Feb 3, 2021
fc00e10
fix: correct the usage of default logger for level and update test
MSNev Feb 4, 2021
a4cb4ba
fix: address additional review comments
MSNev Feb 9, 2021
de8f8ee
Merge branch 'main' into MSNev/AddDiagLogger
dyladan Feb 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ package.json.lerna_backup
# VsCode configs
.vscode/

#Visual Studio
.vs/

#IDEA
.idea
*.iml
6 changes: 3 additions & 3 deletions benchmark/tracer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const benchmark = require('./benchmark');
const opentelemetry = require('../packages/opentelemetry-api');
const { BasicTracerProvider, BatchSpanProcessor, InMemorySpanExporter, SimpleSpanProcessor } = require('../packages/opentelemetry-tracing');

const logger = new opentelemetry.NoopLogger();
const diagLogger = opentelemetry.createNoopDiagLogger();

const setups = [
{
Expand All @@ -13,7 +13,7 @@ const setups = [
},
{
name: 'BasicTracerProvider',
provider: new BasicTracerProvider({ logger })
provider: new BasicTracerProvider({ logger: diagLogger })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a DiagLogger implements the same functions as Logger, this is just updating to show usage and provide a simple concrete compatibility example.

},
{
name: 'BasicTracerProvider with SimpleSpanProcessor',
Expand Down Expand Up @@ -63,7 +63,7 @@ for (const setup of setups) {
suite.run({ async: false });
}
function getProvider(processor) {
const provider = new BasicTracerProvider({ logger });
const provider = new BasicTracerProvider({ logger: diagLogger });
provider.addSpanProcessor(processor);
return provider;
}
Expand Down
7 changes: 5 additions & 2 deletions examples/collector-exporter-node/metrics.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
'use strict';

const { ConsoleLogger, LogLevel } = require('@opentelemetry/core');
const { DiagConsoleLogger, DiagLogLevel, diag } = require('@opentelemetry/api');
const { CollectorMetricExporter } = require('@opentelemetry/exporter-collector');
// const { CollectorMetricExporter } = require('@opentelemetry/exporter-collector-grpc');
// const { CollectorMetricExporter } = require('@opentelemetry/exporter-collector-proto');
const { MeterProvider } = require('@opentelemetry/metrics');

diag.setLogger(new DiagConsoleLogger());
diag.setLogLevel(DiagLogLevel.DEBUG);

const metricExporter = new CollectorMetricExporter({
serviceName: 'basic-metric-service',
// url: 'http://localhost:55681/v1/metrics',
logger: new ConsoleLogger(LogLevel.DEBUG),
vmarchaud marked this conversation as resolved.
Show resolved Hide resolved
logger: diag,
});

const meter = new MeterProvider({
Expand Down
5 changes: 3 additions & 2 deletions examples/collector-exporter-node/tracing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
'use strict';

const opentelemetry = require('@opentelemetry/api');
// const { ConsoleLogger, LogLevel} = require('@opentelemetry/core');
const { BasicTracerProvider, ConsoleSpanExporter, SimpleSpanProcessor } = require('@opentelemetry/tracing');
const { CollectorTraceExporter } = require('@opentelemetry/exporter-collector');
// const { CollectorTraceExporter } = require('@opentelemetry/exporter-collector-grpc');
// const { CollectorTraceExporter } = require('@opentelemetry/exporter-collector-proto');

// opentelemetry.diag.setLogger(new opentelemetry.DiagConsoleLogger());
// opentelemetry.diag.setLogLevel(opentelemetry.DiagLogLevel.DEBUG);

const exporter = new CollectorTraceExporter({
serviceName: 'basic-service',
// logger: new ConsoleLogger(LogLevel.DEBUG),
// headers: {
// foo: 'bar'
// },
Expand Down
4 changes: 2 additions & 2 deletions examples/metrics/metrics/observer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
'use strict';

const { MeterProvider } = require('@opentelemetry/metrics');
const { ConsoleLogger, LogLevel } = require('@opentelemetry/core');
const { DiagConsoleLogger, DiagLogLevel, diagLogLevelFilter } = require('@opentelemetry/api');
const { PrometheusExporter } = require('@opentelemetry/exporter-prometheus');

const exporter = new PrometheusExporter(
Expand Down Expand Up @@ -61,7 +61,7 @@ meter.createBatchObserver((observerBatchResult) => {
});
}, {
maxTimeoutUpdateMS: 500,
logger: new ConsoleLogger(LogLevel.DEBUG)
logger: diagLogLevelFilter(DiagLogLevel.DEBUG, new DiagConsoleLogger())
Copy link
Member

Choose a reason for hiding this comment

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

for me diagLogLevelFilter is confusing. I would move this functionality to chaining

  logger: new DiagConsoleLogger().setLogLevel(DiagConsoleLogger.LOG_LEVEL.DEBUG)

or if we don't want to have static LOG_LEVEL

  logger: new DiagConsoleLogger().setLogLevel(DiagLogLevel.DEBUG)

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 we follow the same factory model (as with the NoopLogger) this would become (perhaps)
logger: createLogLevelDiagLogger(DiagLogLevel.DEBUG, new DiagConsoleLogger())

The DiagnosticAPI does already of a setLogLevel() to enforce a global default logging level. As part of the SDK there is a Global LOG_LEVEL but it hangs off environment so that s a different discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Additionally, once we only use diag this will become

diag.setLogger(new DiagConsoleLogger());
diag.setLogLevel(DiagLogLevel.DEBUG);

And then it would need to be added here.

Copy link
Member

Choose a reason for hiding this comment

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

tbh it looks strange that you still have to create a new DiagConsoleLogger() which you then have to put to the "factory" to create a new LogLevelDiagLogger. Why do I have to create a 2 instances of 2 different classes to be able to have a logger with desired log level.

Things like
logger: new ConsoleLogger(LogLevel.DEBUG)
is self explanatory easy and intuitive, why can't we have
one of:

logger: createDiagConsoleLogger(DiagLogLevel.DEBUG)
// or
logger: new DiagConsoleLogger(DiagLogLevel.DEBUG)
// or
logger: new DiagConsoleLogger().setLogLevel(DiagLogLevel.DEBUG)

comparing to

logger: createLogLevelDiagLogger(DiagLogLevel.DEBUG, new DiagConsoleLogger())

Copy link
Contributor Author

@MSNev MSNev Feb 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is part of the design to reduce code duplication, doing it this way NO DiagLogger (except the DiagLogger returned by createLogLevelDiagLogger()) needs to add any code to enforce log level filtering.

So ALL log level filtering is in a single location and anyone who wants to create their own DiagLogger (file, database, event logger, etc) just doesn't need to worry about it.

The only other solution to achieve the same would be to introduce a Base class that everyone would need to extend and this has a couple of implications, not just on size but the actual function implementation of the Base class would need to be prototype level OR we force every implementor to save / call the base instance level functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And generally, aren't we also saying that the code moving forward (and unless absolutely necessary) would not create it's own instances and instead would use the api.diag instance -- this will be a migration process and

diag.setLogger(new DiagConsoleLogger());
diag.setLogLevel(DiagLogLevel.DEBUG);

Copy link
Member

@dyladan dyladan Feb 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Personally, I think this distinction makes sense and it is similar to previous loggers i've seen. I think confusion may primarily come from the naming. What I have seen in the past is LoggingDestination or LogSink. Here's a very simplified example to help explain what is going on and why it is implemented this way.

// A LogSink is a destination where logs will be sent.
// A LogSink can assume any logs it receives have already had all appropriate filtering applied (e.g. log level filtering)
interface LogSink {
    debug(...args): void;

    info(...args): void;

    warn(...args): void;

    error(...args): void;

    critical(...args): void;
}

enum LogLevel {
    DEBUG = 0,
    INFO = 10,
    WARN = 20,
    ERROR = 30,
    CRITICAL = 40,
}

// Logger class is what users call log methods on.
// It is in charge of doing any required filtering and sending it to the configured sink.
class Logger {
    constructor(
        private level: LogLevel,
        private sink: LogSink,
    ) { }

    debug(...args) {
        if (this.level < LogLevel.DEBUG)
        this.sink.debug(...args)
    }

    info(...args) {
        if (this.level < LogLevel.INFO)
        this.sink.info(...args)
    }

    warn(...args) {
        if (this.level < LogLevel.WARN)
        this.sink.warn(...args)
    }

    error(...args) {
        if (this.level < LogLevel.ERROR)
        this.sink.error(...args)
    }

    critical(...args) {
        if (this.level < LogLevel.CRITICAL)
        this.sink.critical(...args)
    }
}


// console sink does not need to do any filtering. this is already done
class ConsoleLogSink implements LogSink {
    debug(...args) {
        console.log(...args);
    };

    info(...args) {
        console.log(...args);
    };

    warn(...args) {
        console.error(...args);
    };

    error(...args) {
        console.error(...args);
    };

    critical(...args) {
        console.error(...args);
    };
}


// file sink does not need to do any filtering. this is already done
class FileLogSink implements LogSink {
    constructor(private filename: string) { }

    debug(...args) {
        fs.appendFileSync(this.filename, format(...args))
    };

    info(...args) {
        fs.appendFileSync(this.filename, format(...args))
    };

    warn(...args) {
        fs.appendFileSync(this.filename, format(...args))
    };

    error(...args) {
        fs.appendFileSync(this.filename, format(...args))
    };

    critical(...args) {
        fs.appendFileSync(this.filename, format(...args))
    };
}


// User can now use either logging sink/destination without them both having to implement filtering based on log levels
const fileLogger = new Logger(LogLevel.INFO, new FileLogSink("info.log"));
const consoleLogger = new Logger(LogLevel.CRITICAL, new ConsoleLogSink());

Copy link
Member

Choose a reason for hiding this comment

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

thx the "sink" approach / explanation is clear and straightforward

},
);

Expand Down
4 changes: 2 additions & 2 deletions examples/tracer-web/examples/metrics/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
'use strict';

const { ConsoleLogger, LogLevel } = require('@opentelemetry/core');
const { DiagConsoleLogger, DiagLogLevel, diagLogLevelFilter } = require('@opentelemetry/api');
const { CollectorMetricExporter } = require('@opentelemetry/exporter-collector');
const { MeterProvider } = require('@opentelemetry/metrics');

const metricExporter = new CollectorMetricExporter({
serviceName: 'basic-metric-service',
logger: new ConsoleLogger(LogLevel.DEBUG),
logger: diagLogLevelFilter(DiagLogLevel.DEBUG, new DiagConsoleLogger()),
});

let interval;
Expand Down
153 changes: 153 additions & 0 deletions packages/opentelemetry-api/src/api/diag.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
/*
* Copyright The OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import {
DiagLogger,
DiagLogFunction,
createNoopDiagLogger,
diagLoggerFunctions,
} from '../diag/logger';
import { DiagLogLevel, createLogLevelDiagLogger } from '../diag/logLevel';
import {
API_BACKWARDS_COMPATIBILITY_VERSION,
GLOBAL_DIAG_LOGGER_API_KEY,
makeGetter,
_global,
} from './global-utils';

/** Internal simple Noop Diag API that returns a noop logger and does not allow any changes */
function noopDiagApi(): DiagAPI {
const noopApi = createNoopDiagLogger() as DiagAPI;

noopApi.getLogger = () => noopApi;
noopApi.setLogger = noopApi.getLogger;
noopApi.setLogLevel = () => {};

return noopApi;
}

/**
* Singleton object which represents the entry point to the OpenTelemetry internal
* diagnostic API
*/
export class DiagAPI implements DiagLogger {
/** Get the singleton instance of the DiagAPI API */
public static instance(): DiagAPI {
let theInst = null;
if (_global[GLOBAL_DIAG_LOGGER_API_KEY]) {
// Looks like a previous instance was set, so try and fetch it
theInst = _global[GLOBAL_DIAG_LOGGER_API_KEY]?.(
API_BACKWARDS_COMPATIBILITY_VERSION
) as DiagAPI;
}

if (!theInst) {
theInst = new DiagAPI();
_global[GLOBAL_DIAG_LOGGER_API_KEY] = makeGetter(
API_BACKWARDS_COMPATIBILITY_VERSION,
theInst,
noopDiagApi()
);
}

return theInst;
}

/**
* Private internal constructor
* @private
*/
private constructor() {
let _logLevel: DiagLogLevel = DiagLogLevel.INFO;
let _filteredLogger: DiagLogger | null;
let _logger: DiagLogger = createNoopDiagLogger();

function _logProxy(funcName: keyof DiagLogger): DiagLogFunction {
return function () {
const orgArguments = arguments as unknown;
const theLogger = _filteredLogger || _logger;
const theFunc = theLogger[funcName];
if (typeof theFunc === 'function') {
return theFunc.apply(
theLogger,
orgArguments as Parameters<DiagLogFunction>
);
}
};
}

// Using self local variable for minification purposes as 'this' cannot be minified
const self = this;

// DiagAPI specific functions

self.getLogger = (): DiagLogger => {
// Return itself if no existing logger is defined (defaults effectively to a Noop)
return _logger;
};

self.setLogger = (logger: DiagLogger): DiagLogger => {
const prevLogger = _logger;
if (prevLogger !== logger && logger !== self) {
// Simple special case to avoid any possible infinite recursion on the logging functions
_logger = logger || createNoopDiagLogger();
_filteredLogger = createLogLevelDiagLogger(_logLevel, _logger);
}

return prevLogger;
};

self.setLogLevel = (maxLogLevel: DiagLogLevel) => {
if (maxLogLevel !== _logLevel) {
_logLevel = maxLogLevel;
if (_logger) {
_filteredLogger = createLogLevelDiagLogger(maxLogLevel, _logger);
}
}
};

for (let i = 0; i < diagLoggerFunctions.length; i++) {
const name = diagLoggerFunctions[i];
self[name] = _logProxy(name);
}
}

/**
* Return the currently configured logger instance, if no logger has been configured
* it will return itself so any log level filtering will still be applied in this case.
*/
public getLogger!: () => DiagLogger;

/**
* Set the DiagLogger instance
* @param logger - The DiagLogger instance to set as the default logger
* @returns The previously registered DiagLogger
*/
public setLogger!: (logger: DiagLogger) => DiagLogger;

/** Set the default maximum diagnostic logging level */
public setLogLevel!: (maxLogLevel: DiagLogLevel) => void;

// DiagLogger implementation
public verbose!: DiagLogFunction;
public debug!: DiagLogFunction;
public info!: DiagLogFunction;
public warn!: DiagLogFunction;
public startupInfo!: DiagLogFunction;
public error!: DiagLogFunction;
public critical!: DiagLogFunction;
public terminal!: DiagLogFunction;
}
6 changes: 6 additions & 0 deletions packages/opentelemetry-api/src/api/global-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { ContextManager } from '@opentelemetry/context-base';
import { TextMapPropagator } from '../context/propagation/TextMapPropagator';
import { TracerProvider } from '../trace/tracer_provider';
import { _globalThis } from '../platform';
import { DiagAPI } from '../api/diag';

export const GLOBAL_CONTEXT_MANAGER_API_KEY = Symbol.for(
'io.opentelemetry.js.api.context'
Expand All @@ -28,11 +29,16 @@ export const GLOBAL_PROPAGATION_API_KEY = Symbol.for(
);
export const GLOBAL_TRACE_API_KEY = Symbol.for('io.opentelemetry.js.api.trace');

export const GLOBAL_DIAG_LOGGER_API_KEY = Symbol.for(
'io.opentelemetry.js.api.diag'
);

type Get<T> = (version: number) => T;
type OtelGlobal = Partial<{
[GLOBAL_CONTEXT_MANAGER_API_KEY]: Get<ContextManager>;
[GLOBAL_PROPAGATION_API_KEY]: Get<TextMapPropagator>;
[GLOBAL_TRACE_API_KEY]: Get<TracerProvider>;
[GLOBAL_DIAG_LOGGER_API_KEY]: Get<DiagAPI>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created the DiagAPI as the global instance, although this could have some unexpected side effects (which I think is also true for context, propagation and trace.

Specific Scenario:
An application includes multiple "components" with each component importing and using OT... As these are registered on the global instance there will be unexpected interactions between the different components usage of the global API's. This is probably mostly an issue for browser based scenario's vs server side node based etc.

The solution to the above would be to introduce some form of API context that is used and passed around to all API operations etc.

Copy link
Member

Choose a reason for hiding this comment

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

In this case, a mismatched API version should call a noop implementation. This is what the other API types do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which it will as when DiagAPI registers itself it supplies a NoopDiagAPI fallback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the scenario I'm pointing out is that you could have many components using the SAME version of the API, so the Noop doesn't come into play here as everything is registered as a global.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm I wonder if a simpler solution would be to prevent different signals from using different API versions. This is what is proposed in #1772

}>;

export const _global = _globalThis as OtelGlobal;
Expand Down
5 changes: 4 additions & 1 deletion packages/opentelemetry-api/src/common/Logger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@

export type LogFunction = (message: string, ...args: unknown[]) => void;

/** Defines a logger interface. */
/** Defines a logger interface.
* @deprecated This interface will be removed prior to v1.0, use the api.diag
* @see {@link DiagLogger} and {@link DiagAPI}
*/
export interface Logger {
error: LogFunction;
warn: LogFunction;
Expand Down
Loading