Add getters/accessors for readable fields in ReadWriteLogRecord.

[breedx-splk]

Resolves #6895.

To maintain binary compatibility, default implementations were provided.

[breedx-splk]

I'm not sure what we think about this .... it's not entirely necessary, but felt more like duplication without it. If we remove this extension, then we could remove the deprecated getBody() method from this interface, but it feels a tad more fragile to me. If we keep it, then we could probably remove the redundant/duplicated javadoc.

Thoughts?

[robsunday]

I'm not sure about it. On one hand it looks reasonable, on the other hand it seems odd to have toLogRecordData() method in the interface that extends LogRecordData - it is like having a method in subclass that converts it to the superclass. It is a bit confusing to me, so I'd vote for not adding 'extends LogRecordData'.
Actually, maybe we should rename ReadWriteLogRecord to avoid this feeling of parent/child relationship between these two interfaces...

[jack-berg]

I don't think this is correct. We should aim to be symmetric to ReadWriteSpan, which inherits its accessors by extending ReadableSpan.

The notable differences between ReadWriteLogRecord / ReadWriteSpan and LogRecordData / SpanData are:

• The {Signal}Data interfaces are annotated immutable, while the ReadWrite{Signal} are not.
• The {Signal}Data interfaces provide access to Resource while ReadWrite{Signal} do not
• The {Signal}Data interfaces allow you to determine how many attributes were dropped due to limits while ReadWrite{Signal} do not.

In retrospect, maybe there could have been a single interface instead, but now we have to contend with consistency. I think the way to go is to extend the existing patterns consistently, and independently consider whether there's a better API design. But let's separate the issues.

[breedx-splk]

If we're trying to be forcibly consistent between signals, then I think we'd want

public interface ReadWriteLogRecord extends LogRecord, ReadableLogRecord {}

so that we're consistent with

public interface ReadWriteSpan extends Span, ReadableSpan {}

One challenge with that right now is that we have NEITHER LogRecord nor ReadableLogRecord interfaces. For consistency, we would need LogRecord to live in the api module and provide the "write-only" methods and ReadableLogRecord would live in the sdk and provide the getters.

Do we think the the expansion of the logging api is within scope for this?

[jack-berg]

We want to be symmetric to the extent it makes sense. Spans are mutable onStart, and read only onEnd, hence the ReadableLogRecord, ReadWriteLogRecord. The separation of start and end also causes spans to have a dedicated API in Span. These concepts are relevant for logs: Logs constructed and emitted in an atomic operation so no need for a LogRecord interface analogous to Span. And there is only one processor callback which is able to read and write, so no need for a ReadableLogRecord analogous to ReadableSpan.

[breedx-splk]

I think you missed my question, which was "Do we think the expansion of the logging api is within scope for this effort?". This effort being: Adding the ability to get data out of ReadWriteLogRecord without having to convert via toLogRecordData(). I ask this because it appears that we would need to extend the logging api with new interfaces in order to make ReadWriteLogRecord consistent with ReadWriteSpan.

We should aim to be symmetric to ReadWriteSpan,

Right. Sure. I can buy into that idea...but I'm kinda surprised that you started discussing start/end/lifecycle and processor callback concerns as part of this. In this PR I'm only concerned with the data access aspects of these interfaces, and not the lifecycle nor action methods. I think it's super weird that you suggest that ReadableSpan exists only in service of SpanProcessor....let's not redefine "what it is" with "how it is used". It's right there in the name -- a ReadableSpan is a span whose internal data/state can be read. That's all.

I think the way to go is to extend the existing patterns consistently, and independently consider whether there's a better API design.

I'm open to this, but I think it requires adding interfaces in the logging api module. Do you want that as part of this PR?

[jack-berg]

Do we think the expansion of the logging api is within scope for this effort?

No. I think for the purposes of #6895 we want ReadWriteLogRecord to have the capabilities of ReadWriteSpan for reading data without a symmetric interface hierarchy.

In this PR I'm only concerned with the data access aspects of these interfaces, and not the lifecycle nor action methods.

The lifecycle aspects matter because the design of the span interfaces ReadWriteSpan / ReadableSpan / Span directly fall out of the SpanProcessor callbacks and the Span API requirements. Need to compare the LogRecordProcessor callbacks and log API to understand where symmetry makes sense and does not.

I think it's super weird that you suggest that ReadableSpan exists only in service of SpanProcessor....let's not redefine "what it is" with "how it is used". It's right there in the name -- a ReadableSpan is a span whose internal data/state can be read.

The only place ReadableSpan occurs in the SDK's API is in SpanProcessor#onEnd. If SpanProcessor didn't have a requirement for a read-only span onEnd, ReadableSpan wouldn't exist. And so we named it something appropriate reflecting "what it is", but we can't loose sight of "why it exists". I don't think it makes sense to invent interfaces that don't serve any expressed purpose in the API.

public interface ReadWriteLogRecord extends LogRecordData

One additional comment on the original proposal: we can't have ReadWriteLogRecord extend LogRecordData because LogRecordData is annotated @Immutable, and ReadWriteLogRecord violates this.

[codecov]

Codecov Report

Attention: Patch coverage is 4.76190% with 20 lines in your changes missing coverage. Please review.

Project coverage is 90.01%. Comparing base (efdacc1) to head (a934242).
Report is 19 commits behind head on main.

Files with missing lines	Patch %	Lines
.../opentelemetry/sdk/logs/SdkReadWriteLogRecord.java	8.33%	11 Missing ⚠️
.../io/opentelemetry/sdk/logs/ReadWriteLogRecord.java	0.00%	9 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6924      +/-   ##
============================================
- Coverage     90.09%   90.01%   -0.09%     
- Complexity     6601     6602       +1     
============================================
  Files           730      730              
  Lines         19843    19864      +21     
  Branches       1955     1956       +1     
============================================
+ Hits          17877    17880       +3     
- Misses         1371     1389      +18     
  Partials        595      595              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[robsunday]

I'm not sure about it. On one hand it looks reasonable, on the other hand it seems odd to have toLogRecordData() method in the interface that extends LogRecordData - it is like having a method in subclass that converts it to the superclass. It is a bit confusing to me, so I'd vote for not adding 'extends LogRecordData'.
Actually, maybe we should rename ReadWriteLogRecord to avoid this feeling of parent/child relationship between these two interfaces...

[robsunday]

Maybe it would be worth adding some unit tests for these default methods? It could be beneficial for possible future classes that do not overwrite them (or overwrite them partially)

[jack-berg]

Just two small additional comments. Looks good to me otherwise.