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
+67 −14
Merged

Add data field to the Event interface #4575

Show file tree
Hide file tree
Changes from 5 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
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
9 changes: 9 additions & 0 deletions 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 @@ -15,6 +15,9 @@
*/

import { Attributes } from '@opentelemetry/api';
import { LogAttributeValue } from '@opentelemetry/api-logs';

export type EventDataValue = LogAttributeValue;

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

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

/**
* 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"
}
]
}
2 changes: 1 addition & 1 deletion 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 @@ -73,7 +73,7 @@ export interface LogRecord {
/**
* A value containing the body of the log record.
*/
body?: string;
body?: LogAttributeValue;
martinkuba marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

@pichlermarc pichlermarc Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's also a little confusing when we use the type LogAttributeValue for the body the logs bridge API. Should we use something named LogRecordBody (or similar) instead? Another option may be to create a type AnyValue and have LogAttributeValue be an alias to that. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LogAttributeValue is currently defined along with LogAttributes like this:

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

I like the idea of introducing AnyValue type as part of the common API, but then we would need to also define something in place of LogAttributes, maybe AnyValues or AnyValueMap?

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

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. Yes, I think the option you proposed with AnyValue and AnyValueMap works best.

Looked at another repo: In Java, they call it KeyAnyValueList, leaning on the name from protobuf. That's unfortunately not applicable for us the attributes are not organized as a list. KeyAnyValueMap would feel redundant as it's implicit that there's a key in a map. So I'm leaning towards AnyValueMap as you suggested. 🙂

Copy link
Contributor Author

@martinkuba martinkuba Mar 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I have added the AnyValue and AnyValueMap types to the core API. I think this a breaking change for the API, and I have updated its changelog accordingly.

I have also changed the LogRecord body type to LogBody. I considered LogRecordBody, but we already used LogAttributes for attributes, so I wanted to be consistent.


/**
* 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, LogAttributeValue } 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?: LogAttributeValue;
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: LogAttributeValue | undefined) {
if (this._isLogRecordReadonly()) {
return;
}
this._body = body;
}
get body(): string | undefined {
get body(): LogAttributeValue | 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 {
LogAttributeValue,
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?: LogAttributeValue;
readonly resource: IResource;
readonly instrumentationScope: InstrumentationScope;
readonly attributes: LogAttributes;
Expand Down