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): split out event api #3501

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
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 .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ logs
npm-debug.log*
yarn-debug.log*
yarn-error.log*
# Filter Logs singal files
!experimental/examples/logs
!experimental/packages/otlp-transformer/src/logs
Copy link
Member

Choose a reason for hiding this comment

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

Why are we filtering in the src directory? Could easily see this backfiring or being confusing if someone tries to make a logs transformer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why are we filtering in the src directory? Could easily see this backfiring or being confusing if someone tries to make a logs transformer.

Sorry, this is something I forgot to remove when I was doing the sdk implementation validation, I have removed it


# Runtime data
pids
Expand Down
2 changes: 0 additions & 2 deletions experimental/packages/api-logs/src/NoopLogger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,8 @@
*/

import { Logger } from './types/Logger';
import { LogEvent } from './types/LogEvent';
import { LogRecord } from './types/LogRecord';

export class NoopLogger implements Logger {
emitLogRecord(_logRecord: LogRecord): void {}
emitEvent(_event: LogEvent): void {}
}
3 changes: 2 additions & 1 deletion experimental/packages/api-logs/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,10 @@
*/

export * from './types/Logger';
export * from './types/EventLogger';
export * from './types/LoggerProvider';
export * from './types/EventLoggerProvider';
export * from './types/LogRecord';
export * from './types/LogEvent';
export * from './types/LoggerOptions';

import { LogsAPI } from './api/logs';
Expand Down
27 changes: 27 additions & 0 deletions experimental/packages/api-logs/src/types/EventLogger.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* 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 { LogRecord } from './LogRecord';

export interface EventLogger {
/**
* Emit a log event.
*
* @param eventName the Event name.
* @param logRecord the LogRecord representing the Event.
*/
emitLogEvent(eventName: string, logRecord: LogRecord): void;
Copy link
Member

Choose a reason for hiding this comment

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

Spec uses the name 'emit event' and I think keeping log out of the name makes it less confusing.

Copy link
Member

Choose a reason for hiding this comment

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

Are we using the log record type intentionally? Seems like it should have an event type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spec uses the name 'emit event' and I think keeping log out of the name makes it less confusing.

This is strictly in accordance with the specification. In fact, the specification recommends "logEvent". But it's just a suggestion. Maybe what you said is right. I have changed it to "emitEvent".
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we using the log record type intentionally? Seems like it should have an event type?

As above, here is strictly in accordance with the specification too, these two guys go together. If we don't highlight the log, we really need an Event Type. But the two of them are actually one data model. I added a 'type EventRecord = LogRecord' and change api to emitEvent(eventName: string, eventRecord: EventRecord): void;, here can discuss whether the eventName should be placed in the EventRecord type

}
32 changes: 32 additions & 0 deletions experimental/packages/api-logs/src/types/EventLoggerProvider.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* 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 { EventLogger } from './EventLogger';
import { Logger } from './Logger';
import { LoggerProvider } from './LoggerProvider';

/**
* A registry for creating named {@link EventLogger}s.
*/
export interface EventLoggerProvider extends LoggerProvider {
/**
* Returns a EventLogger instance and is responsible for emitting Events as LogRecords.
*
* @param logger the delegate Logger used to emit Events as LogRecords.
* @param domain the domain of emitted events, used to set the event.domain attribute.
*/
getEventLogger(logger: Logger, domain: string): EventLogger;
}
54 changes: 0 additions & 54 deletions experimental/packages/api-logs/src/types/LogEvent.ts

This file was deleted.

61 changes: 42 additions & 19 deletions experimental/packages/api-logs/src/types/LogRecord.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,56 @@
* limitations under the License.
*/

import { Attributes } from '@opentelemetry/api';
import { Attributes, Context, TimeInput } from '@opentelemetry/api';

export enum SeverityNumber {
UNSPECIFIED = 0,
TRACE = 1,
TRACE2 = 2,
TRACE3 = 3,
TRACE4 = 4,
DEBUG = 5,
DEBUG2 = 6,
DEBUG3 = 7,
DEBUG4 = 8,
INFO = 9,
INFO2 = 10,
INFO3 = 11,
INFO4 = 12,
WARN = 13,
WARN2 = 14,
WARN3 = 15,
WARN4 = 16,
ERROR = 17,
ERROR2 = 18,
ERROR3 = 19,
ERROR4 = 20,
FATAL = 21,
FATAL2 = 22,
FATAL3 = 23,
FATAL4 = 24,
}

export interface LogRecord {
/**
* The time when the log record occurred as UNIX Epoch time in nanoseconds.
* The time when the log record occurred
*/
time?: TimeInput;

/**
* The time when the log record was observed by the collection system
*/
timestamp?: number;
observedTime?: TimeInput;

/**
* The Context including TraceContext of the log record
*/
context?: Context;

/**
* Numerical value of the severity.
*/
severityNumber?: number;
severityNumber?: SeverityNumber;

/**
* The severity text.
Expand All @@ -41,19 +79,4 @@ export interface LogRecord {
* Attributes that define the log record.
*/
attributes?: Attributes;

/**
* 8 least significant bits are the trace flags as defined in W3C Trace Context specification.
*/
traceFlags?: number;

/**
* A unique identifier for a trace.
*/
traceId?: string;

/**
* A unique identifier for a span within a trace.
*/
spanId?: string;
}
8 changes: 0 additions & 8 deletions experimental/packages/api-logs/src/types/Logger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
*/

import { LogRecord } from './LogRecord';
import { LogEvent } from './LogEvent';

export interface Logger {
/**
Expand All @@ -24,11 +23,4 @@ export interface Logger {
* @param logRecord
*/
emitLogRecord(logRecord: LogRecord): void;

/**
* Emit an event. This method should only be used by instrumentations emitting events.
*
* @param event
*/
emitEvent(event: LogEvent): void;
}
12 changes: 3 additions & 9 deletions experimental/packages/api-logs/src/types/LoggerOptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,10 @@ export interface LoggerOptions {
schemaUrl?: string;

/**
* The default domain for events created by the Logger.
*
* The combination of event name and event domain uiquely identifies an event.
* By supplying an event domain, it is possible to use the same event name across
* different domains / use cases.
*
* The default domain can be overridden when emitting an individual event.
* @default ''
* Specifies whether the Trace Context should automatically be passed on to the LogRecords emitted by the Logger.
* @default true
*/
eventDomain?: string;
includeTraceContext?: boolean;

/**
* The instrumentation scope attributes to associate with emitted telemetry
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/

import * as assert from 'assert';
import { SeverityNumber } from '../../src';
import { NoopLogger } from '../../src/NoopLogger';
import { NoopLoggerProvider } from '../../src/NoopLoggerProvider';

Expand All @@ -24,13 +25,11 @@ describe('NoopLogger', () => {
assert(logger instanceof NoopLogger);
});

it('calling emitEvent should not crash', () => {
const logger = new NoopLoggerProvider().getLogger('test-noop');
logger.emitEvent({ name: 'event-name', domain: 'event-domain' });
});

it('calling emitLogRecord should not crash', () => {
const logger = new NoopLoggerProvider().getLogger('test-noop');
logger.emitLogRecord({ severityNumber: 1, body: 'log body' });
logger.emitLogRecord({
severityNumber: SeverityNumber.TRACE,
body: 'log body',
});
});
});
1 change: 1 addition & 0 deletions lerna.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
{
"version": "independent",
"npmClient": "npm",
"useNx": false,
"packages": [
"api",
"packages/*",
Expand Down