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

feat: add OTEL_LOG_LEVEL env var #974

Merged
merged 5 commits into from
Jul 27, 2020
Merged

feat: add OTEL_LOG_LEVEL env var #974

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
33f7a04
feat: add OTEL_LOG_LEVEL env var
Apr 18, 2020
8b313d5
fix: typo
Jul 25, 2020
03bc84d
test: add one for mock environment that is now shared in 2 test files
Jul 27, 2020
9934e58
fix: revert exporting test function in sdk
Jul 27, 2020
878dc39
Merge branch 'master' into otel-log-level
Jul 27, 2020
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: 2 additions & 1 deletion packages/opentelemetry-core/src/common/ConsoleLogger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,10 @@

import { Logger } from '@opentelemetry/api';
import { LogLevel } from './types';
import { getEnv } from '../platform';

export class ConsoleLogger implements Logger {
constructor(level: LogLevel = LogLevel.INFO) {
constructor(level: LogLevel = getEnv().OTEL_LOG_LEVEL) {
if (level >= LogLevel.DEBUG) {
this.debug = (...args) => {
console.debug(...args);
Expand Down
6 changes: 6 additions & 0 deletions packages/opentelemetry-core/src/common/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ export enum LogLevel {
DEBUG,
}

/**
* This is equivalent to:
* type LogLevelString = 'ERROR' | 'WARN' | 'INFO' | 'DEBUG';
*/
export type LogLevelString = keyof typeof LogLevel;
Copy link
Contributor

Choose a reason for hiding this comment

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

Very cool you can do this!

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 really happy to stumble upon it here https://www.typescriptlang.org/docs/handbook/enums.html#enums-at-compile-time 😄


/**
* This interface defines a fallback to read a timeOrigin when it is not available on performance.timeOrigin,
* this happens for example on Safari Mac
Expand Down
74 changes: 71 additions & 3 deletions packages/opentelemetry-core/src/utils/environment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ export interface ENVIRONMENT {
}

const ENVIRONMENT_NUMBERS: Partial<keyof ENVIRONMENT>[] = [
'OTEL_LOG_LEVEL',
'OTEL_SAMPLING_PROBABILITY',
];

Expand All @@ -37,7 +36,7 @@ const ENVIRONMENT_NUMBERS: Partial<keyof ENVIRONMENT>[] = [
*/
export const DEFAULT_ENVIRONMENT: Required<ENVIRONMENT> = {
OTEL_NO_PATCH_MODULES: '',
OTEL_LOG_LEVEL: LogLevel.ERROR,
OTEL_LOG_LEVEL: LogLevel.INFO,
obecny marked this conversation as resolved.
Show resolved Hide resolved
OTEL_SAMPLING_PROBABILITY: 1,
};

Expand All @@ -64,6 +63,41 @@ function parseNumber(
}
}

/**
* Environmentally sets log level if valid log level string is provided
* @param key
* @param environment
* @param values
*/
function setLogLevelFromEnv(
key: keyof ENVIRONMENT,
environment: ENVIRONMENT_MAP | ENVIRONMENT,
values: ENVIRONMENT_MAP
) {
const value = values[key];
switch (typeof value === 'string' ? value.toUpperCase() : value) {
case 'DEBUG':
environment[key] = LogLevel.DEBUG;
break;

case 'INFO':
environment[key] = LogLevel.INFO;
break;

case 'WARN':
environment[key] = LogLevel.WARN;
break;

case 'ERROR':
environment[key] = LogLevel.ERROR;
break;

default:
// do nothing
break;
}
}

/**
* Parses environment values
* @param values
Expand All @@ -79,7 +113,7 @@ export function parseEnvironment(values: ENVIRONMENT_MAP): ENVIRONMENT {
break;

case 'OTEL_LOG_LEVEL':
parseNumber(key, environment, values, LogLevel.ERROR, LogLevel.DEBUG);
setLogLevelFromEnv(key, environment, values);
break;

default:
Expand All @@ -95,3 +129,37 @@ export function parseEnvironment(values: ENVIRONMENT_MAP): ENVIRONMENT {

return environment;
}

let lastMock: ENVIRONMENT_MAP = {};

/**
* Mocks environment used for tests.
*/
export function mockEnvironment(values: ENVIRONMENT_MAP) {
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason you move it ?. keeping them just in tests it is fine, but exporting them as our sdk api with checking if process exists might not be a good solution Whatever we export to be a public must be platform dependent. If you are doing a helper function for you tests it is fine, but not when you try to export it.

Copy link
Member

Choose a reason for hiding this comment

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

@dyladan ^^

Copy link
Member Author

Choose a reason for hiding this comment

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

So the idea was to use it in multiple test files. While moving it here and exporting it so that multiple test files could use it seems to work it might not be the best solution.

We had a test-utils package but that was moved to contrib repo I believe. Should I copy paste these functions in both test files?

Copy link
Member

Choose a reason for hiding this comment

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

but it was inside file /test/utils/environment.test.ts which is perfectly fine, by moving this to environment.ts mean we are exporting a test function as our api which is not something we want

Copy link
Member

Choose a reason for hiding this comment

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

test-utils is in the contrib repo because it was only used by contrib plugins, but it might be a good idea to move it back into the main repo. I have said on other PRs I don't like the idea of exporting testing/fake/mock behavior from modules because users will find them and they will use them even if we dont want them to, and they will be confused and we will have to deal with it.

Copy link
Member

Choose a reason for hiding this comment

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

so you can still easily share this function between tests in the same package, just don't export them as our sdk

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 I will change accordingly as not to export the function

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, exported from its original location (test file itself) not sure why i did not do that first time around, think I was fatigued at that time.

Copy link
Member

Choose a reason for hiding this comment

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

just fix the conflicts pls

lastMock = values;
if (typeof process !== 'undefined') {
Object.keys(values).forEach(key => {
process.env[key] = String(values[key]);
});
} else {
Object.keys(values).forEach(key => {
((window as unknown) as ENVIRONMENT_MAP)[key] = String(values[key]);
});
}
}

/**
* Removes mocked environment used for tests.
*/
export function removeMockEnvironment() {
if (typeof process !== 'undefined') {
Object.keys(lastMock).forEach(key => {
delete process.env[key];
});
} else {
Object.keys(lastMock).forEach(key => {
delete ((window as unknown) as ENVIRONMENT_MAP)[key];
});
}
lastMock = {};
}
37 changes: 37 additions & 0 deletions packages/opentelemetry-core/test/common/ConsoleLogger.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@
import * as assert from 'assert';
import { ConsoleLogger } from '../../src/common/ConsoleLogger';
import { LogLevel } from '../../src/common/types';
import {
removeMockEnvironment,
mockEnvironment,
} from '../../src/utils/environment';

describe('ConsoleLogger', () => {
const origDebug = console.debug;
Expand Down Expand Up @@ -54,6 +58,7 @@ describe('ConsoleLogger', () => {
console.info = origInfo;
console.warn = origWarn;
console.error = origError;
removeMockEnvironment();
});

describe('constructor', () => {
Expand All @@ -65,6 +70,8 @@ describe('ConsoleLogger', () => {
assert.deepStrictEqual(warnCalledArgs, ['warn called %s', 'param1']);
consoleLogger.info('info called %s', 'param1');
assert.deepStrictEqual(infoCalledArgs, ['info called %s', 'param1']);
consoleLogger.debug('debug called %s', 'param1');
assert.strictEqual(debugCalledArgs, undefined);
});

it('should log with debug', () => {
Expand Down Expand Up @@ -114,5 +121,35 @@ describe('ConsoleLogger', () => {
consoleLogger.debug('debug called %s', 'param1');
assert.strictEqual(debugCalledArgs, null);
});

it('should log with environmentally set level ', () => {
mockEnvironment({
OTEL_LOG_LEVEL: 'WARN',
});
const consoleLogger = new ConsoleLogger();
consoleLogger.error('error called');
assert.deepStrictEqual(errorCalledArgs, ['error called']);
consoleLogger.warn('warn called %s', 'param1');
assert.deepStrictEqual(warnCalledArgs, ['warn called %s', 'param1']);
consoleLogger.info('info called %s', 'param1');
assert.deepStrictEqual(infoCalledArgs, null);
consoleLogger.debug('debug called %s', 'param1');
assert.deepStrictEqual(debugCalledArgs, null);
});

it('should log with default log level if environmentally set level is invalid', () => {
mockEnvironment({
OTEL_LOG_LEVEL: 'INVALID_VALUE',
});
const consoleLogger = new ConsoleLogger();
consoleLogger.error('error called');
assert.deepStrictEqual(errorCalledArgs, ['error called']);
consoleLogger.warn('warn called %s', 'param1');
assert.deepStrictEqual(warnCalledArgs, ['warn called %s', 'param1']);
consoleLogger.info('info called %s', 'param1');
assert.deepStrictEqual(infoCalledArgs, ['info called %s', 'param1']);
consoleLogger.debug('debug called %s', 'param1');
assert.deepStrictEqual(debugCalledArgs, null);
});
});
});
57 changes: 26 additions & 31 deletions packages/opentelemetry-core/test/utils/environment.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,38 +18,12 @@ import { getEnv } from '../../src/platform';
import {
DEFAULT_ENVIRONMENT,
ENVIRONMENT,
ENVIRONMENT_MAP,
removeMockEnvironment,
mockEnvironment,
} from '../../src/utils/environment';
import * as assert from 'assert';
import * as sinon from 'sinon';

let lastMock: ENVIRONMENT_MAP = {};

function mockEnvironment(values: ENVIRONMENT_MAP) {
lastMock = values;
if (typeof process !== 'undefined') {
Object.keys(values).forEach(key => {
process.env[key] = String(values[key]);
});
} else {
Object.keys(values).forEach(key => {
((window as unknown) as ENVIRONMENT_MAP)[key] = String(values[key]);
});
}
}

function removeMockEnvironment() {
if (typeof process !== 'undefined') {
Object.keys(lastMock).forEach(key => {
delete process.env[key];
});
} else {
Object.keys(lastMock).forEach(key => {
delete ((window as unknown) as ENVIRONMENT_MAP)[key];
});
}
lastMock = {};
}
import { LogLevel } from '../../src';

describe('environment', () => {
let sandbox: sinon.SinonSandbox;
Expand All @@ -68,15 +42,23 @@ describe('environment', () => {
mockEnvironment({
FOO: '1',
OTEL_NO_PATCH_MODULES: 'a,b,c',
OTEL_LOG_LEVEL: '1',
OTEL_LOG_LEVEL: 'ERROR',
OTEL_SAMPLING_PROBABILITY: '0.5',
});
const env = getEnv();
assert.strictEqual(env.OTEL_NO_PATCH_MODULES, 'a,b,c');
assert.strictEqual(env.OTEL_LOG_LEVEL, 1);
assert.strictEqual(env.OTEL_LOG_LEVEL, LogLevel.ERROR);
assert.strictEqual(env.OTEL_SAMPLING_PROBABILITY, 0.5);
});

it('should parse OTEL_LOG_LEVEL despite casing', () => {
mockEnvironment({
OTEL_LOG_LEVEL: 'waRn',
});
const env = getEnv();
assert.strictEqual(env.OTEL_LOG_LEVEL, LogLevel.WARN);
});

it('should parse environment variables and use defaults', () => {
const env = getEnv();
Object.keys(DEFAULT_ENVIRONMENT).forEach(envKey => {
Expand All @@ -89,4 +71,17 @@ describe('environment', () => {
});
});
});

describe('mockEnvironment', () => {
it('should remove a mock environment', () => {
mockEnvironment({
OTEL_LOG_LEVEL: 'DEBUG',
OTEL_SAMPLING_PROBABILITY: 0.5,
});
removeMockEnvironment();
const env = getEnv();
assert.strictEqual(env.OTEL_LOG_LEVEL, LogLevel.INFO);
assert.strictEqual(env.OTEL_SAMPLING_PROBABILITY, 1);
});
});
});
4 changes: 2 additions & 2 deletions packages/opentelemetry-metrics/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import { LogLevel } from '@opentelemetry/core';
import { LogLevel, getEnv } from '@opentelemetry/core';
import * as api from '@opentelemetry/api';
import { MetricExporter } from './export/types';
import { Resource } from '@opentelemetry/resources';
Expand Down Expand Up @@ -43,7 +43,7 @@ export interface MeterConfig {

/** Default Meter configuration. */
export const DEFAULT_CONFIG = {
logLevel: LogLevel.INFO,
logLevel: getEnv().OTEL_LOG_LEVEL,
};

/** The default metric creation options value. */
Expand Down
4 changes: 2 additions & 2 deletions packages/opentelemetry-tracing/src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import { AlwaysOnSampler, LogLevel } from '@opentelemetry/core';
import { AlwaysOnSampler, getEnv } from '@opentelemetry/core';

/** Default limit for Message events per span */
export const DEFAULT_MAX_EVENTS_PER_SPAN = 128;
Expand All @@ -31,7 +31,7 @@ export const DEFAULT_MAX_LINKS_PER_SPAN = 32;
*/
export const DEFAULT_CONFIG = {
defaultAttributes: {},
logLevel: LogLevel.INFO,
logLevel: getEnv().OTEL_LOG_LEVEL,
sampler: new AlwaysOnSampler(),
traceParams: {
numberOfAttributesPerSpan: DEFAULT_MAX_ATTRIBUTES_PER_SPAN,
Expand Down