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

Add getters/accessors for readable fields in ReadWriteLogRecord. #6924

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

breedx-splk
Copy link
Contributor

Resolves #6895.

To maintain binary compatibility, default implementations were provided.

@breedx-splk breedx-splk requested a review from a team as a code owner December 6, 2024 00:37

/**
* A log record that can be read from and written to.
*
* @since 1.27.0
*/
public interface ReadWriteLogRecord {
public interface ReadWriteLogRecord extends LogRecordData {
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'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?

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

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 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?

Copy link

codecov bot commented Dec 6, 2024

Codecov Report

Attention: Patch coverage is 0% with 29 lines in your changes missing coverage. Please review.

Project coverage is 89.97%. Comparing base (efdacc1) to head (23c1c1b).

Files with missing lines Patch % Lines
.../opentelemetry/sdk/logs/SdkReadWriteLogRecord.java 0.00% 17 Missing ⚠️
.../io/opentelemetry/sdk/logs/ReadWriteLogRecord.java 0.00% 12 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6924      +/-   ##
============================================
- Coverage     90.09%   89.97%   -0.12%     
  Complexity     6601     6601              
============================================
  Files           730      730              
  Lines         19843    19872      +29     
  Branches       1955     1957       +2     
============================================
+ Hits          17877    17880       +3     
- Misses         1371     1398      +27     
+ Partials        595      594       -1     

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


/**
* A log record that can be read from and written to.
*
* @since 1.27.0
*/
public interface ReadWriteLogRecord {
public interface ReadWriteLogRecord extends LogRecordData {

Choose a reason for hiding this comment

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

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

*/
@Nullable
default <T> T getAttribute(AttributeKey<T> key) {
return toLogRecordData().getAttributes().get(key);

Choose a reason for hiding this comment

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

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ReadWriteLogRecord accessors
3 participants