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.

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

feat(api-logs): Add delegating no-op logger provider #4861

Merged
merged 15 commits into from
Sep 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions experimental/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ All notable changes to experimental packages in this project will be documented

### :rocket: (Enhancement)

* feat(api-logs): Add delegating no-op logger provider [#4861](https://github.com/open-telemetry/opentelemetry-js/pull/4861) @hectorhdzg
Copy link
Contributor

Choose a reason for hiding this comment

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

[marylia]

Hey Hector, I see that you marked this as "Breaking change", would you mind providing more info on that?

[Hector]

this is a breaking change because the behavior of logs.getLoggerProvider will return a ProxyLoggerProvider instead of the actual registered logger provider, same as current behavior for tracerProvider

Is that really a breaking change? logs.getLoggerProvider returns an object of type interface LoggerProvider both before and after, so I wonder (and hope) this could be considered not a breaking change.

A breaking change isn't so big a deal for this package, because it is still 0.x. However, a breaking change for the delegating MeterProvider in #4858, which is part of the "api" package, would be a very big deal: there is a VERY strong disincentive to have a major version bump for the "api" package.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hectorhdzg We discussed in the SIG today (I brought it up). We agreed that we do not need to consider this a breaking change, because the exported types for these APIs are interfaces.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @trentm would update changelog then


### :bug: (Bug Fix)

* fix(sampler-jaeger-remote): fixes an issue where package could emit unhandled promise rejections @Just-Sieb
Expand Down
69 changes: 69 additions & 0 deletions experimental/packages/api-logs/src/ProxyLogger.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/*
* 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 { NOOP_LOGGER } from './NoopLogger';
import { Logger } from './types/Logger';
import { LoggerOptions } from './types/LoggerOptions';
import { LogRecord } from './types/LogRecord';

export class ProxyLogger implements Logger {
// When a real implementation is provided, this will be it
private _delegate?: Logger;

constructor(
private _provider: LoggerDelegator,
public readonly name: string,
public readonly version?: string | undefined,
public readonly options?: LoggerOptions | undefined
) {}

/**
* Emit a log record. This method should only be used by log appenders.
*
* @param logRecord
*/
emit(logRecord: LogRecord): void {
this._getLogger().emit(logRecord);
}

/**
* Try to get a logger from the proxy logger provider.
* If the proxy logger provider has no delegate, return a noop logger.
*/
private _getLogger() {
if (this._delegate) {
return this._delegate;
}
const logger = this._provider.getDelegateLogger(
this.name,
this.version,
this.options
);
if (!logger) {
return NOOP_LOGGER;
}
this._delegate = logger;
return this._delegate;
}
}

export interface LoggerDelegator {
getDelegateLogger(
name: string,
version?: string,
options?: LoggerOptions
): Logger | undefined;
}
55 changes: 55 additions & 0 deletions experimental/packages/api-logs/src/ProxyLoggerProvider.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/*
* 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 { LoggerProvider } from './types/LoggerProvider';
import { Logger } from './types/Logger';
import { LoggerOptions } from './types/LoggerOptions';
import { NOOP_LOGGER_PROVIDER } from './NoopLoggerProvider';
import { ProxyLogger } from './ProxyLogger';

export class ProxyLoggerProvider implements LoggerProvider {
private _delegate?: LoggerProvider;

getLogger(
name: string,
version?: string | undefined,
options?: LoggerOptions | undefined
): Logger {
return (
this.getDelegateLogger(name, version, options) ??
new ProxyLogger(this, name, version, options)
);
}

getDelegate(): LoggerProvider {
return this._delegate ?? NOOP_LOGGER_PROVIDER;
}

/**
* Set the delegate logger provider
*/
setDelegate(delegate: LoggerProvider) {
this._delegate = delegate;
}

getDelegateLogger(
name: string,
version?: string | undefined,
options?: LoggerOptions | undefined
): Logger | undefined {
return this._delegate?.getLogger(name, version, options);
}
}
7 changes: 6 additions & 1 deletion experimental/packages/api-logs/src/api/logs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,13 @@ import { LoggerProvider } from '../types/LoggerProvider';
import { NOOP_LOGGER_PROVIDER } from '../NoopLoggerProvider';
import { Logger } from '../types/Logger';
import { LoggerOptions } from '../types/LoggerOptions';
import { ProxyLoggerProvider } from '../ProxyLoggerProvider';

export class LogsAPI {
private static _instance?: LogsAPI;

private _proxyLoggerProvider = new ProxyLoggerProvider();

private constructor() {}

public static getInstance(): LogsAPI {
Expand All @@ -48,6 +51,7 @@ export class LogsAPI {
provider,
NOOP_LOGGER_PROVIDER
);
this._proxyLoggerProvider.setDelegate(provider);

return provider;
Copy link
Contributor

Choose a reason for hiding this comment

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

Aside: I was comparing this setGlobal... to the equivalent for the TracerProvider and noticed that the internal/global-utils.ts and global registration handling is very different in api-logs from what it is in the "api" package. Apparently the code here was copied from api-metrics when that was a separate package ... but before the api-metrics package's global-registration handling was changed to #3357

When the api-logs package is merged into the "api" package, it will have to adapt to this new global-utils handling. That could be done before or after this PR. Until then, the global registration handling is subtley different between them.

(api-events will want to change as well)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I was planning to take care of that with api-logs migration to api package PR but that one got complicated, I will take care of using same global utils in different PR.

}
Expand All @@ -60,7 +64,7 @@ export class LogsAPI {
public getLoggerProvider(): LoggerProvider {
return (
_global[GLOBAL_LOGS_API_KEY]?.(API_BACKWARDS_COMPATIBILITY_VERSION) ??
NOOP_LOGGER_PROVIDER
this._proxyLoggerProvider
);
}

Expand All @@ -80,5 +84,6 @@ export class LogsAPI {
/** Remove the global logger provider */
public disable(): void {
delete _global[GLOBAL_LOGS_API_KEY];
this._proxyLoggerProvider = new ProxyLoggerProvider();
}
}
2 changes: 2 additions & 0 deletions experimental/packages/api-logs/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ export { LoggerOptions } from './types/LoggerOptions';
export { AnyValue, AnyValueMap } from './types/AnyValue';
export { NOOP_LOGGER, NoopLogger } from './NoopLogger';
export { NOOP_LOGGER_PROVIDER, NoopLoggerProvider } from './NoopLoggerProvider';
export { ProxyLogger } from './ProxyLogger';
export { ProxyLoggerProvider } from './ProxyLoggerProvider';

import { LogsAPI } from './api/logs';
export const logs = LogsAPI.getInstance();
10 changes: 6 additions & 4 deletions experimental/packages/api-logs/test/api/api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,19 @@
*/

import * as assert from 'assert';
import { Logger, logs } from '../../src';
import { Logger, ProxyLoggerProvider, logs } from '../../src';
import { NoopLogger } from '../../src/NoopLogger';
import { NoopLoggerProvider } from '../../src/NoopLoggerProvider';

describe('API', () => {
const dummyLogger = new NoopLogger();

it('should expose a logger provider via getLoggerProvider', () => {
const provider = logs.getLoggerProvider();
assert.ok(provider);
assert.strictEqual(typeof provider, 'object');
assert.ok(logs.getLoggerProvider() instanceof ProxyLoggerProvider);
assert.ok(
(logs.getLoggerProvider() as ProxyLoggerProvider).getDelegate() instanceof
NoopLoggerProvider
);
});

describe('GlobalLoggerProvider', () => {
Expand Down
8 changes: 4 additions & 4 deletions experimental/packages/api-logs/test/internal/global.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import * as assert from 'assert';
import { _global, GLOBAL_LOGS_API_KEY } from '../../src/internal/global-utils';
import { NoopLoggerProvider } from '../../src/NoopLoggerProvider';
import { ProxyLoggerProvider } from '../../src';

const api1 = require('../../src') as typeof import('../../src');

Expand Down Expand Up @@ -66,11 +67,10 @@ describe('Global Utils', () => {
assert.strictEqual(original, api1.logs.getLoggerProvider());
});

it('should return the module NoOp implementation if the version is a mismatch', () => {
const original = api1.logs.getLoggerProvider();
api1.logs.setGlobalLoggerProvider(new NoopLoggerProvider());
it('should return the module no op implementation if the version is a mismatch', () => {
api1.logs.setGlobalLoggerProvider(new ProxyLoggerProvider());
const afterSet = _global[GLOBAL_LOGS_API_KEY]!(-1);

assert.strictEqual(original, afterSet);
assert.ok(afterSet instanceof NoopLoggerProvider);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
/*
* 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 * as assert from 'assert';
import * as sinon from 'sinon';
import {
Logger,
LoggerProvider,
ProxyLogger,
ProxyLoggerProvider,
} from '../../src';
import { NoopLogger } from '../../src/NoopLogger';

describe('ProxyLogger', () => {
let provider: ProxyLoggerProvider;
const sandbox = sinon.createSandbox();

beforeEach(() => {
provider = new ProxyLoggerProvider();
});

afterEach(() => {
sandbox.restore();
});

describe('when no delegate is set', () => {
it('should return proxy loggers', () => {
const logger = provider.getLogger('test');
assert.ok(logger instanceof ProxyLogger);
});
});

describe('when delegate is set before getLogger', () => {
let delegate: LoggerProvider;
let getLoggerStub: sinon.SinonStub;

beforeEach(() => {
getLoggerStub = sandbox.stub().returns(new NoopLogger());
delegate = {
getLogger: getLoggerStub,
};
provider.setDelegate(delegate);
});

it('should return loggers directly from the delegate', () => {
const logger = provider.getLogger('test', 'v0');

sandbox.assert.calledOnce(getLoggerStub);
assert.strictEqual(getLoggerStub.firstCall.returnValue, logger);
assert.deepStrictEqual(getLoggerStub.firstCall.args, [
'test',
'v0',
undefined,
]);
});

it('should return loggers directly from the delegate with schema url', () => {
const logger = provider.getLogger('test', 'v0', {
schemaUrl: 'https://opentelemetry.io/schemas/1.7.0',
});

sandbox.assert.calledOnce(getLoggerStub);
assert.strictEqual(getLoggerStub.firstCall.returnValue, logger);
assert.deepStrictEqual(getLoggerStub.firstCall.args, [
'test',
'v0',
{ schemaUrl: 'https://opentelemetry.io/schemas/1.7.0' },
]);
});
});

describe('when delegate is set after getLogger', () => {
let logger: Logger;
let delegateProvider: LoggerProvider;

let delegateLogger: Logger;
let emitCalled: boolean;

beforeEach(() => {
emitCalled = false;
delegateLogger = {
emit() {
emitCalled = true;
},
};

logger = provider.getLogger('test');

delegateProvider = {
getLogger() {
return delegateLogger;
},
};
provider.setDelegate(delegateProvider);
});

it('should emit from the delegate logger', () => {
logger.emit({
body: 'Test',
});
assert.ok(emitCalled);
});
});
});
Loading