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

Reduce OpenTelemetry warnings #1125

Merged
merged 3 commits into from
Jul 11, 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
3 changes: 3 additions & 0 deletions packages/opentelemetry/lib/config/instrumentations.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ exports.createInstrumentationConfig = function createInstrumentationConfig() {
},
'@opentelemetry/instrumentation-fs': {
enabled: false
},
'@opentelemetry/instrumentation-pino': {
enabled: false
}
}),
new RuntimeNodeInstrumentation()
Expand Down
31 changes: 17 additions & 14 deletions packages/opentelemetry/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const logger = require('@dotcom-reliability-kit/logger');

/**
* @import { Instances, Options } from '@dotcom-reliability-kit/opentelemetry'
* @import { MeterProvider } from '@opentelemetry/sdk-metrics'
*/

/**
Expand Down Expand Up @@ -44,19 +45,18 @@ function setupOpenTelemetry({
});
}

// Use a Reliability Kit logger for logging. The DiagLogLevel
// does nothing here – Reliability Kit's log level (set through
// the LOG_LEVEL environment variable) takes over. We set the
// OpenTelemetry log level to the maximum value that we want
// Reliability Kit to consider logging
opentelemetry.api.diag.setLogger(
// @ts-ignore this complains because DiagLogger accepts a type
// of unknown whereas our logger is stricter. This is fine though,
// if something unknown is logged then we do our best with it.
// It's easier to ignore this error than fix it.
logger.createChildLogger({ event: 'OTEL_INTERNALS' }),
opentelemetry.api.DiagLogLevel.INFO
);
// We need to create a new logger for OpenTelemetry internals
// because the function signature is different and OpenTelemetry
// logs mutliple strings. With pino this means we lose data
const log = logger.createChildLogger({ event: 'OTEL_INTERNALS' });
const opentelemetryLogger = {
error: (...args) => log.error(args[0], { details: args.slice(1) }),
info: (...args) => log.info(args[0], { details: args.slice(1) }),
warn: (...args) => log.warn(args[0], { details: args.slice(1) }),
debug: () => {},
verbose: () => {}
};
opentelemetry.api.diag.setLogger(opentelemetryLogger);

// Set up and start OpenTelemetry
instances = {
Expand All @@ -80,7 +80,10 @@ function setupOpenTelemetry({

// Set up host metrics if we have a metrics endpoint
if (metricsOptions?.endpoint) {
instances.hostMetrics = new HostMetrics();
const meterProvider = /** @type {MeterProvider} */ (
opentelemetry.api.metrics.getMeterProvider()
);
instances.hostMetrics = new HostMetrics({ meterProvider });
instances.hostMetrics.start();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ describe('@dotcom-reliability-kit/opentelemetry/lib/config/instrumentation', ()
},
'@opentelemetry/instrumentation-fs': {
enabled: false
},
'@opentelemetry/instrumentation-pino': {
enabled: false
}
});
});
Expand Down
79 changes: 74 additions & 5 deletions packages/opentelemetry/test/unit/lib/index.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ describe('@dotcom-reliability-kit/opentelemetry', () => {
let createTracingConfig;
let HostMetrics;
let logger;
let mockChildLogger;
let NodeSDK;
let opentelemetry;

Expand All @@ -42,11 +43,19 @@ describe('@dotcom-reliability-kit/opentelemetry', () => {
createTracingConfig =
require('../../../lib/config/tracing').createTracingConfig;
api = require('@opentelemetry/sdk-node').api;
api.metrics.getMeterProvider.mockReturnValue('mock-meter-provider');
HostMetrics = require('@opentelemetry/host-metrics').HostMetrics;
NodeSDK = require('@opentelemetry/sdk-node').NodeSDK;
logger = require('@dotcom-reliability-kit/logger');

logger.createChildLogger.mockReturnValue('mock child logger');
mockChildLogger = {
debug: jest.fn(),
error: jest.fn(),
info: jest.fn(),
verbose: jest.fn(),
warn: jest.fn()
};
logger.createChildLogger.mockReturnValue(mockChildLogger);
api.DiagLogLevel.INFO = 'mock info log level';

opentelemetry = require('../../../lib/index');
Expand All @@ -69,18 +78,76 @@ describe('@dotcom-reliability-kit/opentelemetry', () => {
});
});

it('sets up OpenTelemetry to log via Reliability Kit logger', () => {
it('sets up OpenTelemetry to log via a custom logger', () => {
expect(logger.createChildLogger).toHaveBeenCalledTimes(1);
expect(logger.createChildLogger).toHaveBeenCalledWith({
event: 'OTEL_INTERNALS'
});
expect(api.diag.setLogger).toHaveBeenCalledTimes(1);
expect(api.diag.setLogger).not.toHaveBeenCalledWith(mockChildLogger);
expect(api.diag.setLogger).toHaveBeenCalledWith(
'mock child logger',
'mock info log level'
expect.objectContaining({
debug: expect.any(Function),
error: expect.any(Function),
info: expect.any(Function),
verbose: expect.any(Function),
warn: expect.any(Function)
})
);
});

describe('custom logger', () => {
let customLogger;

beforeEach(() => {
customLogger = api.diag.setLogger.mock.calls[0][0];
});

describe('.debug()', () => {
it('does nothing', () => {
customLogger.debug('mock message 1', 'mock message 2');
expect(mockChildLogger.debug).toHaveBeenCalledTimes(0);
});
});

describe('.error()', () => {
it('logs an error via the Reliability Kit child logger', () => {
customLogger.error('mock message 1', 'mock message 2');
expect(mockChildLogger.error).toHaveBeenCalledTimes(1);
expect(mockChildLogger.error).toHaveBeenCalledWith('mock message 1', {
details: ['mock message 2']
});
});
});

describe('.info()', () => {
it('logs info via the Reliability Kit child logger', () => {
customLogger.info('mock message 1', 'mock message 2');
expect(mockChildLogger.info).toHaveBeenCalledTimes(1);
expect(mockChildLogger.info).toHaveBeenCalledWith('mock message 1', {
details: ['mock message 2']
});
});
});

describe('.verbose()', () => {
it('does nothing', () => {
customLogger.verbose('mock message 1', 'mock message 2');
expect(mockChildLogger.verbose).toHaveBeenCalledTimes(0);
});
});

describe('.warn()', () => {
it('logs an warning via the Reliability Kit child logger', () => {
customLogger.warn('mock message 1', 'mock message 2');
expect(mockChildLogger.warn).toHaveBeenCalledTimes(1);
expect(mockChildLogger.warn).toHaveBeenCalledWith('mock message 1', {
details: ['mock message 2']
});
});
});
});

it('configures the OpenTelemetry instrumentations', () => {
expect(createInstrumentationConfig).toHaveBeenCalledTimes(1);
expect(createInstrumentationConfig).toHaveBeenCalledWith();
Expand Down Expand Up @@ -119,7 +186,9 @@ describe('@dotcom-reliability-kit/opentelemetry', () => {

it('instantiates and starts a HostMetrics collector', () => {
expect(HostMetrics).toHaveBeenCalledTimes(1);
expect(HostMetrics).toHaveBeenCalledWith();
expect(HostMetrics).toHaveBeenCalledWith({
meterProvider: 'mock-meter-provider'
});
expect(HostMetrics.prototype.start).toHaveBeenCalledTimes(1);
});

Expand Down
Loading