Add data field to the Event interface

[martinkuba]

Which problem is this PR solving?

Fixes #4570

The specification for the Event API includes payload field which is mapped to the body field of LogRecord. This is described in semantic conventions here.

This also requires changing the type of LogRecord body field from string to AnyValue (implemented as LogAttributeValue).

• New feature (non-breaking change which adds functionality)

[codecov]

Codecov Report

Merging #4575 (5c5678d) into main (f3aedb7) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4575      +/-   ##
==========================================
+ Coverage   92.85%   92.86%   +0.01%     
==========================================
  Files         328      328              
  Lines        9499     9499              
  Branches     2042     2042              
==========================================
+ Hits         8820     8821       +1     
+ Misses        679      678       -1     

Files	Coverage Δ
...erimental/packages/api-logs/src/types/LogRecord.ts	100.00% <ø> (ø)
experimental/packages/sdk-logs/src/LogRecord.ts	98.05% <100.00%> (ø)

... and 1 file with indirect coverage changes

[pichlermarc]

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. 🤔

[martinkuba]

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;
}

[pichlermarc]

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. 🙂

[martinkuba]

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.

[pichlermarc]

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).

[martinkuba]

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?

[martinkuba]

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

[pichlermarc]

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 🙂 )