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

Add data field to the Event interface #4575

Merged
merged 12 commits into from
Apr 2, 2024
Merged

Add data field to the Event interface #4575

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
2b59dad
add `data` field to the Event interface
martinkuba Mar 22, 2024
1342cd0
updated body field in the Logs SDK
martinkuba Mar 22, 2024
a302e9e
updated changelog to breaking change
martinkuba Mar 22, 2024
f0cc15e
lint
martinkuba Mar 22, 2024
0053c00
added dedicated type for event data field
martinkuba Mar 25, 2024
0149ed2
added AnyValue and AnyValueMap types for Event data
martinkuba Mar 28, 2024
a33b8e8
changed body type to LogBody
martinkuba Mar 28, 2024
e5fa009
markdown lint
martinkuba Mar 28, 2024
60e5810
updated Logs SDK
martinkuba Mar 28, 2024
f215a17
changed to non-breaking change in the core API
martinkuba Mar 29, 2024
712493e
moved AnyValue to Log API, updated changelog
martinkuba Mar 29, 2024
5c5678d
Merge branch 'main' into event-payload
martinkuba Mar 30, 2024
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 api/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ All notable changes to this project will be documented in this file.

### :boom: Breaking Change

* feat(api)!: add AnyValue and AnyValueMap types [#4575](https://github.com/open-telemetry/opentelemetry-js/pull/4575)
Copy link
Member

@pichlermarc pichlermarc Mar 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, would you mind elaborating why this would be a breaking change (did you perhaps mean to mark it as breaking for the Logs API)? 🤔

Since we're just adding types that don't need to be implemented by anyone it should not affect users or implementors (we've done this in the past as non-breaking when adding the Metrics API).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that adding any new export is a breaking change, but I was not 100% sure. There are three changes:

  • adding the two types to the core API
  • adding a new field to the experimental Event interface
  • changing type of an existing field in the LogRecord interface (and Log SDK implementations)

Is only the last one a breaking change? Should I capture these as three different bullet points in the changelog?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed the API changelog to a non-breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

Adding a new export should be fine. 🙂

Thinking about it though, if we add it to the core API now, it's immediately stable, so we might want to get a few more eyes on it.

To move ahead with this PR quicker, I'd suggest we export the AnyValue and AnyValueMap types from @opentelemetry/api-logs for now, as we'll integrate that it into @opentelemetry/api eventually when we declare it as stable. At this point it'll already be tried and tested, and it'll give us some flexibility in changing them until it's time to mark them as stable.

A side note: what's usually breaking for implementors (like this repo or dd-trace as a third party implementation) is if we add a non-optional field/method to an existing interface. It's not breaking for consumers of the API as they can just simply use more of the API without needing to worry that it's undefined.

  • adding a new field to the experimental Event interface
    • as it's optional, this is non-breaking for both implementors and consumers
  • changing type of an existing field in the LogRecord interface (and Log SDK implementations)
    • this is breaking for implementors (they will now recieve types in their implementation that they did not expect before, and type-checking will fail), but not users (they can still pass string to it just fine 🙂 )


### :rocket: (Enhancement)

### :bug: (Bug Fix)
Expand Down
12 changes: 12 additions & 0 deletions api/src/common/Attributes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,15 @@ export type AttributeValue =
| Array<null | undefined | string>
| Array<null | undefined | number>
| Array<null | undefined | boolean>;

/**
* AnyValueMap is a map from string to AnyValue (attribute value or a nested map)
*/
export interface AnyValueMap {
[attributeKey: string]: AnyValue | undefined;
}

/**
* AnyValue is a either an attribute value or a map of AnyValue(s)
*/
export type AnyValue = AttributeValue | AnyValueMap;
7 changes: 6 additions & 1 deletion api/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,12 @@ export { BaggageEntry, BaggageEntryMetadata, Baggage } from './baggage/types';
export { baggageEntryMetadataFromString } from './baggage/utils';
export { Exception } from './common/Exception';
export { HrTime, TimeInput } from './common/Time';
export { Attributes, AttributeValue } from './common/Attributes';
export {
Attributes,
AttributeValue,
AnyValue,
AnyValueMap,
} from './common/Attributes';

// Context APIs
export { createContextKey, ROOT_CONTEXT } from './context/context';
Expand Down
1 change: 1 addition & 0 deletions experimental/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ All notable changes to experimental packages in this project will be documented
* was used internally to keep track of the compression to use but was unintentionally exposed to the users. It allowed to read and write the value, writing, however, would have no effect.
* feat(api-events)!: removed domain from the Events API [#4569](https://github.com/open-telemetry/opentelemetry-js/pull/4569)
* fix(events-api)!: renamed EventEmitter to EventLogger in the Events API [#4569](https://github.com/open-telemetry/opentelemetry-js/pull/4568)
* feat(api-events)!: added data field to the Event interface [4575](https://github.com/open-telemetry/opentelemetry-js/pull/4575)

### :rocket: (Enhancement)

Expand Down
3 changes: 2 additions & 1 deletion experimental/packages/api-events/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@
"access": "public"
},
"dependencies": {
"@opentelemetry/api": "^1.0.0"
"@opentelemetry/api": "^1.0.0",
"@opentelemetry/api-logs": "0.49.1"
},
"devDependencies": {
"@types/mocha": "10.0.6",
Expand Down
8 changes: 7 additions & 1 deletion experimental/packages/api-events/src/types/Event.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 { Attributes } from '@opentelemetry/api';
import { AnyValue, Attributes } from '@opentelemetry/api';

export interface Event {
/**
Expand All @@ -27,6 +27,12 @@ export interface Event {
*/
name: string;

/**
* Data that describes the event.
* Intended to be used by instrumentation libraries.
*/
data?: AnyValue;

/**
* Additional attributes that describe the event.
*/
Expand Down
3 changes: 3 additions & 0 deletions experimental/packages/api-events/tsconfig.esm.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@
"references": [
{
"path": "../../../api"
},
{
"path": "../api-logs"
}
]
}
3 changes: 3 additions & 0 deletions experimental/packages/api-events/tsconfig.esnext.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@
"references": [
{
"path": "../../../api"
},
{
"path": "../api-logs"
}
]
}
3 changes: 3 additions & 0 deletions experimental/packages/api-events/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
"references": [
{
"path": "../../../api"
},
{
"path": "../api-logs"
}
]
}
10 changes: 4 additions & 6 deletions experimental/packages/api-logs/src/types/LogRecord.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,10 @@
* limitations under the License.
*/

import { AttributeValue, Context, TimeInput } from '@opentelemetry/api';
import { AnyValue, AnyValueMap, Context, TimeInput } from '@opentelemetry/api';

export type LogAttributeValue = AttributeValue | LogAttributes;
export interface LogAttributes {
[attributeKey: string]: LogAttributeValue | undefined;
}
export type LogBody = AnyValue;
export type LogAttributes = AnyValueMap;

export enum SeverityNumber {
UNSPECIFIED = 0,
Expand Down Expand Up @@ -73,7 +71,7 @@ export interface LogRecord {
/**
* A value containing the body of the log record.
*/
body?: string;
body?: LogBody;

/**
* Attributes that define the log record.
Expand Down
8 changes: 4 additions & 4 deletions experimental/packages/sdk-logs/src/LogRecord.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import type { IResource } from '@opentelemetry/resources';

import type { ReadableLogRecord } from './export/ReadableLogRecord';
import type { LogRecordLimits } from './types';
import { LogAttributes } from '@opentelemetry/api-logs';
import { LogAttributes, LogBody } from '@opentelemetry/api-logs';
import { LoggerProviderSharedState } from './internal/LoggerProviderSharedState';

export class LogRecord implements ReadableLogRecord {
Expand All @@ -38,7 +38,7 @@ export class LogRecord implements ReadableLogRecord {
readonly attributes: logsAPI.LogAttributes = {};
private _severityText?: string;
private _severityNumber?: logsAPI.SeverityNumber;
private _body?: string;
private _body?: LogBody;
private totalAttributesCount: number = 0;

private _isReadonly: boolean = false;
Expand All @@ -64,13 +64,13 @@ export class LogRecord implements ReadableLogRecord {
return this._severityNumber;
}

set body(body: string | undefined) {
set body(body: LogBody | undefined) {
if (this._isLogRecordReadonly()) {
return;
}
this._body = body;
}
get body(): string | undefined {
get body(): LogBody | undefined {
return this._body;
}

Expand Down
8 changes: 6 additions & 2 deletions experimental/packages/sdk-logs/src/export/ReadableLogRecord.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,19 @@
import type { IResource } from '@opentelemetry/resources';
import type { HrTime, SpanContext } from '@opentelemetry/api';
import type { InstrumentationScope } from '@opentelemetry/core';
import type { LogAttributes, SeverityNumber } from '@opentelemetry/api-logs';
import type {
LogBody,
LogAttributes,
SeverityNumber,
} from '@opentelemetry/api-logs';

export interface ReadableLogRecord {
readonly hrTime: HrTime;
readonly hrTimeObserved: HrTime;
readonly spanContext?: SpanContext;
readonly severityText?: string;
readonly severityNumber?: SeverityNumber;
readonly body?: string;
readonly body?: LogBody;
readonly resource: IResource;
readonly instrumentationScope: InstrumentationScope;
readonly attributes: LogAttributes;
Expand Down