Skip to content
This repository has been archived by the owner on Nov 10, 2022. It is now read-only.

chore: use semver to determine API compatibility #6

Closed
wants to merge 10 commits into from
Closed

chore: use semver to determine API compatibility #6

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
73bd4da
chore: use semver to determine API compatibility
dyladan Feb 19, 2021
3c9b9e7
chore: do not throw
dyladan Feb 19, 2021
2b576fa
chore: only filtered logger is global
dyladan Feb 23, 2021
29e47ff
chore: fix typo
dyladan Feb 23, 2021
04dda0e
chore: always use level set by user
dyladan Feb 23, 2021
d728462
Merge pull request #1 from dyladan/semver-compatibility-no-level
dyladan Feb 23, 2021
28b2f3e
chore: prevent infinite recursion
dyladan Feb 23, 2021
640c1ea
chore: rename destination child
dyladan Feb 23, 2021
ca1b381
chore: review comments
dyladan Feb 24, 2021
a6ede59
chore: fix build and add fallback
dyladan Feb 24, 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
1 change: 1 addition & 0 deletions .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ module.exports = {
"files": ["test/**/*.ts"],
"rules": {
"no-empty": "off",
"@typescript-eslint/ban-ts-comment": "off",
Copy link
Member Author

Choose a reason for hiding this comment

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

We need ts comments for asserting a type check will fail. This allows it in tests only

"@typescript-eslint/ban-ts-ignore": "off",
"@typescript-eslint/no-empty-function": "off",
"@typescript-eslint/no-explicit-any": "off",
Expand Down
30 changes: 8 additions & 22 deletions src/api/context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@
import { NoopContextManager } from '../context/NoopContextManager';
import { Context, ContextManager } from '../context/types';
import {
API_BACKWARDS_COMPATIBILITY_VERSION,
GLOBAL_CONTEXT_MANAGER_API_KEY,
makeGetter,
_global,
} from './global-utils';
getGlobal,
registerGlobal,
unregisterGlobal,
} from '../internal/global-utils';

const API_NAME = 'context';
const NOOP_CONTEXT_MANAGER = new NoopContextManager();

/**
Expand All @@ -49,17 +49,7 @@ export class ContextAPI {
public setGlobalContextManager(
contextManager: ContextManager
): ContextManager {
if (_global[GLOBAL_CONTEXT_MANAGER_API_KEY]) {
// global context manager has already been set
return this._getContextManager();
}

_global[GLOBAL_CONTEXT_MANAGER_API_KEY] = makeGetter(
API_BACKWARDS_COMPATIBILITY_VERSION,
contextManager,
NOOP_CONTEXT_MANAGER
);

registerGlobal(API_NAME, contextManager);
return contextManager;
}

Expand Down Expand Up @@ -98,16 +88,12 @@ export class ContextAPI {
}

private _getContextManager(): ContextManager {
return (
_global[GLOBAL_CONTEXT_MANAGER_API_KEY]?.(
API_BACKWARDS_COMPATIBILITY_VERSION
) ?? NOOP_CONTEXT_MANAGER
);
return getGlobal(API_NAME) || NOOP_CONTEXT_MANAGER;
}

/** Disable and remove the global context manager */
public disable() {
this._getContextManager().disable();
delete _global[GLOBAL_CONTEXT_MANAGER_API_KEY];
unregisterGlobal(API_NAME);
}
}
95 changes: 33 additions & 62 deletions src/api/diag.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,66 +19,42 @@ import {
DiagLogFunction,
createNoopDiagLogger,
diagLoggerFunctions,
FilteredDiagLogger,
} 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 {
dyladan marked this conversation as resolved.
Show resolved Hide resolved
const noopApi = createNoopDiagLogger() as DiagAPI;

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

return noopApi;
}
getGlobal,
registerGlobal,
unregisterGlobal,
} from '../internal/global-utils';

/**
* Singleton object which represents the entry point to the OpenTelemetry internal
* diagnostic API
*/
export class DiagAPI implements DiagLogger {
private static _instance?: DiagAPI;

/** 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()
);
if (!this._instance) {
this._instance = new DiagAPI();
}

return theInst;
return this._instance;
}

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

function _logProxy(funcName: keyof DiagLogger): DiagLogFunction {
return function () {
const orgArguments = arguments as unknown;
const theLogger = _filteredLogger || _logger;
const theLogger = self.getLogger();
const theFunc = theLogger[funcName];
if (typeof theFunc === 'function') {
return theFunc.apply(
Expand All @@ -94,29 +70,20 @@ export class DiagAPI implements DiagLogger {

// DiagAPI specific functions

self.getLogger = (): DiagLogger => {
// Return itself if no existing logger is defined (defaults effectively to a Noop)
return _logger;
self.getLogger = (): FilteredDiagLogger => {
return getGlobal('diag') || _noopLogger;
Copy link
Contributor

Choose a reason for hiding this comment

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

Thought: Do we even need this anymore?

Can we just have this as the getChildLogger(), as there is no need to call getLogger() to bypass the filtering (as it won't) and the DiagAPI is a logger so you don't need to call api.diag.getLogger().debug(...)

Copy link
Contributor

Choose a reason for hiding this comment

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

Especially, as this getLogger() was previously just here to return the child logger.

Copy link
Contributor

Choose a reason for hiding this comment

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

And this would mean that the usage on L56 would have to change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we need the diag API to understand the concept of filtering at all? We could just have the DiagLogger interface, and require the user to construct a valid logger for their use case like diag.setLogger(logLevelFilteredLogger(level, console))

Copy link
Contributor

Choose a reason for hiding this comment

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

The approach I was taking is that users and creators of loggers shouldn't need to worry about log level filtering as this should be controlled by the DiagAPI and any initialization configuration (as it should only be set once).

So I prefer what you have done in the setLogger registerGlobal('diag', createLogLevelDiagLogger(logLevel, logger), true) as it continues to hide the complexity.

};

self.setLogger = (logger?: DiagLogger): DiagLogger => {
const prevLogger = _logger;
if (!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.setLogger = (
logger: DiagLogger = _noopLogger,
logLevel: DiagLogLevel = DiagLogLevel.INFO
) => {
logger = logger === self ? self.getLogger().getChild() : logger;
registerGlobal('diag', createLogLevelDiagLogger(logLevel, logger), true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth adding a comment that we are allowing the override of the default to both replacing any existing logger AND to avoid any possible endless loop (because of someone refactoring the code as the existing implementation doesn't have an issue) -- just so it make anyone touching the code in the future to think about that scenario before just blindly changing it.

};

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

for (let i = 0; i < diagLoggerFunctions.length; i++) {
Expand All @@ -129,22 +96,26 @@ export class DiagAPI implements DiagLogger {
* 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;
public getLogger!: () => FilteredDiagLogger;

/**
* Set the DiagLogger instance
* @param logger - [Optional] The DiagLogger instance to set as the default logger, if not provided it will set it back as a noop
* Set the global DiagLogger and DiagLogLevel
*
* @param logger - [Optional] The DiagLogger instance to set as the default logger. If not provided it will set it back as a noop.
* @param logLevel - [Optional] The DiagLogLevel used to filter logs sent to the logger. If not provided it will default to INFO.
* @returns The previously registered DiagLogger
*/
public setLogger!: (logger?: DiagLogger) => DiagLogger;

/** Set the default maximum diagnostic logging level */
public setLogLevel!: (maxLogLevel: DiagLogLevel) => void;
public setLogger!: (logger?: DiagLogger, logLevel?: DiagLogLevel) => void;

// DiagLogger implementation
public verbose!: DiagLogFunction;
public debug!: DiagLogFunction;
public info!: DiagLogFunction;
public warn!: DiagLogFunction;
public error!: DiagLogFunction;

/**
* Unregister the global logger and return to Noop
*/
public disable!: () => void;
}
70 changes: 0 additions & 70 deletions src/api/global-utils.ts
View file
Open in desktop

This file was deleted.

31 changes: 9 additions & 22 deletions src/api/propagation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,12 @@ import {
TextMapSetter,
} from '../propagation/TextMapPropagator';
import {
API_BACKWARDS_COMPATIBILITY_VERSION,
GLOBAL_PROPAGATION_API_KEY,
makeGetter,
_global,
} from './global-utils';
getGlobal,
registerGlobal,
unregisterGlobal,
} from '../internal/global-utils';

const API_NAME = 'propagation';

/**
* Singleton object which represents the entry point to the OpenTelemetry Propagation API
Expand All @@ -52,17 +53,7 @@ export class PropagationAPI {
* Set the current propagator. Returns the initialized propagator
*/
public setGlobalPropagator(propagator: TextMapPropagator): TextMapPropagator {
if (_global[GLOBAL_PROPAGATION_API_KEY]) {
// global propagator has already been set
return this._getGlobalPropagator();
}

_global[GLOBAL_PROPAGATION_API_KEY] = makeGetter(
API_BACKWARDS_COMPATIBILITY_VERSION,
propagator,
NOOP_TEXT_MAP_PROPAGATOR
);

registerGlobal(API_NAME, propagator);
return propagator;
}

Expand Down Expand Up @@ -105,14 +96,10 @@ export class PropagationAPI {

/** Remove the global propagator */
public disable() {
delete _global[GLOBAL_PROPAGATION_API_KEY];
unregisterGlobal(API_NAME);
}

private _getGlobalPropagator(): TextMapPropagator {
return (
_global[GLOBAL_PROPAGATION_API_KEY]?.(
API_BACKWARDS_COMPATIBILITY_VERSION
) ?? NOOP_TEXT_MAP_PROPAGATOR
);
return getGlobal(API_NAME) || NOOP_TEXT_MAP_PROPAGATOR;
}
}
Loading