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

Experimental support for Log AnyValue body #5880

Merged
merged 7 commits into from
Oct 31, 2023

Conversation

jack-berg
Copy link
Member

@jack-berg jack-berg commented Oct 4, 2023

Resolves #5847.

This adds experimental support for recording AnyValue for log record bodies. A summary:

  • (almost) All changes are in the incubator package for now
  • Add a new interface called AnyValue which mirrors the proto AnyValue definition.
  • Add a overload of LogRecordBuilder#setBody(..) which accepts AnyValue as an argument.
  • In the SDK, extend Body.Type enum with a new value ANY_VALUE (other values are STRING and EMPTY). Unfortunately, while Body is aligned with the new AnyValue (it has a getType() method and could theoretically be extended to represent arbitrarily complex data) the name isn't right. It would be weird to have a body nested within a body nested within a body. So Body appears destined to be a wrapper around AnyValue. This was an API design miss in the log SDK. Still, the ergonomics shouldn't be too bad.
  • To use, exporters should check the Body#getType(). If it is AnyValue, they can cast it to AnyValueBody, access the AnyValue, and encode it however they see fit. If the exporter protocol doesn't understand how to export AnyValueBody, it can use Body#asString(), which will encode the AnyValue data into a String. The AnyValueBody#asString() implementation currently produces a java-toString style response, but we could substitute a JSON encoding to make it even more useful.

Will need to followup on this to update LogMarshaler to properly encode this to OTLP. Currently, it will just send to the asString() representation.

@codecov
Copy link

codecov bot commented Oct 4, 2023

@jackshirazi
Copy link
Contributor

jackshirazi commented Oct 12, 2023

Is this effectively a new serialization type? What is the advantage over declaring a new interface (like

interface LogBody { byte[] convertToBytes(); }

or even just allowing Externalizable objects or an equivalent interface?

@jack-berg
Copy link
Member Author

@jackshirazi the log record body field is defined in the data model as type any. See proto definition for body, and AnyValue.

So far, we've only known about use cases where log record body is a string, so we've restricted it. However, new use cases have emerged which require the log record body to include structure:

  • Structured logs in slf4j / logback / log4j. We're currently mapping the key value pairs of the structured log to attributes, but this isn't quite right - they really represent the body of the log in the same way a string message. Also, mapping the structured log key value pairs to the body ensures there aren't any conflicts with MDC keys, which we map to attributes.
  • Events. We want to support events with complex types. When mapping events from other systems, its important to keep the structure as close as possible to the original, and AnyValue facilitates that.

So its not as simple as having a type that can be serializes to byte[] - we need to know about the structure / types of the fields so we can encode it to proto AnyValue.

@jack-berg
Copy link
Member Author

I haven't heard any criticism of this approach so I've gone ahead and tidied it up and marked it ready for review.

@jack-berg jack-berg marked this pull request as ready for review October 13, 2023 20:57
@jack-berg jack-berg requested a review from a team October 13, 2023 20:57
@jack-berg jack-berg changed the title Log AnyValue body prototype Experimental support for Log AnyValue body Oct 13, 2023
Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

Hey @jack-berg this is great! I had a few comments/questions, but overall this feels solid to me.

public byte[] getValue() {
return value;
public ByteBuffer getValue() {
return ByteBuffer.wrap(raw).asReadOnlyBuffer();
Copy link
Contributor

Choose a reason for hiding this comment

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

Stoked!

assertThat(anyValue.getValue()).isEqualTo(new byte[] {'a', 'b'});
ByteBuffer value = anyValue.getValue();
// AnyValueBytes returns read only view of ByteBuffer
assertThatThrownBy(value::array).isInstanceOf(ReadOnlyBufferException.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻

@jack-berg
Copy link
Member Author

@open-telemetry/java-approvers PTAL when you have a chance. I've got a followup PR drafted in #5938 which extends the OTLP marshalers to be able to handle AnyValue. Would be good to bundle this and #5938 in the same release.

@breedx-splk
Copy link
Contributor

This also very much relates to the ongoing discussions around event.data vs. this style of event body. See open-telemetry/opentelemetry-android#122 and the working group open-telemetry/community#1702 and https://github.com/orgs/open-telemetry/projects/65/views/1

@jack-berg
Copy link
Member Author

Will merge tomorrow if there are no additional comments. Need to leave time for the followup #5938 before the next release.

@jack-berg
Copy link
Member Author

In the interest of being very conservative, I've removed the new Body#ANY_VALUE enum option. There is now no new public API surface area.

Anyone wishing to access the AnyValue in an LogRecordExporter should check if the if(logRecord.getBody() instanceof AnyValueBody).

@jack-berg jack-berg merged commit efa46a5 into open-telemetry:main Oct 31, 2023
18 checks passed
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.

Allow Log API to record structured body
3 participants