Stabilize log any value #6591

jack-berg · 2024-07-17T18:34:09Z

Resolves #6581. Unblocks #6509.

Promote AnyValue from the incubating artifact to the stable API. Moved it to the io.opentelemetry.api.common package because while its only usage is in the log API today, its possible that future otel APIs reuse the more generic AnyValue representations.
On the SDK side, deprecate the Body class. It serves the same function as AnyValue, but really should have been in the API package from the start. Since its not, retaining Body and extending it to represent AnyValue concepts results in a cumbersome API with an unnecessary layer of abstraction (i.e. Body encloses AnyValue encloses your data). Instead we have a new LogRecordData#getAnyValueBody() method which represents all the different body types. Steps are taken to ensure users relying on LogRecordData#getBody() are not impacted, and continue to get a functional string representation even when the body is an AnyValue.

codecov · 2024-07-17T18:50:31Z

Codecov Report

Attention: Patch coverage is 81.39535% with 32 lines in your changes missing coverage. Please review.

Project coverage is 90.03%. Comparing base (7522bfe) to head (9764054).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
...ntelemetry/sdk/testing/logs/TestLogRecordData.java	10.00%	9 Missing ⚠️
...va/io/opentelemetry/sdk/logs/SdkLogRecordData.java	0.00%	4 Missing ⚠️
...java/io/opentelemetry/api/common/KeyValueList.java	86.95%	1 Missing and 2 partials ⚠️
...n/java/io/opentelemetry/api/common/ValueArray.java	81.25%	1 Missing and 2 partials ⚠️
...va/io/opentelemetry/api/logs/LogRecordBuilder.java	0.00%	2 Missing ⚠️
...etry/exporter/internal/otlp/AnyValueMarshaler.java	77.77%	1 Missing and 1 partial ⚠️
.../io/opentelemetry/sdk/logs/data/LogRecordData.java	0.00%	2 Missing ⚠️
...java/io/opentelemetry/api/common/ValueBoolean.java	80.00%	0 Missing and 1 partial ⚠️
...n/java/io/opentelemetry/api/common/ValueBytes.java	80.00%	0 Missing and 1 partial ⚠️
.../java/io/opentelemetry/api/common/ValueDouble.java	80.00%	0 Missing and 1 partial ⚠️
... and 4 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6591      +/-   ##
============================================
- Coverage     90.09%   90.03%   -0.06%     
+ Complexity     6280     6277       -3     
============================================
  Files           697      697              
  Lines         18951    18939      -12     
  Branches       1858     1864       +6     
============================================
- Hits          17073    17051      -22     
- Misses         1305     1314       +9     
- Partials        573      574       +1

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

trask

can we avoid the deprecations by adding an AnyValueBody subtype of Body?

sdk/testing/src/main/java/io/opentelemetry/sdk/testing/logs/TestLogRecordData.java

jack-berg · 2024-07-18T14:49:19Z

can we avoid the deprecations by adding an AnyValueBody subtype of Body?

This is what I was originally intending on doing, but see my comment in the PR description:

On the SDK side, deprecate the Body class. It serves the same function as AnyValue, but really should have been in the API package from the start. Since its not, retaining Body and extending it to represent AnyValue concepts results in a cumbersome API with an unnecessary layer of abstraction (i.e. Body encloses AnyValue encloses your data). Instead we have a new LogRecordData#getAnyValueBody() method which represents all the different body types. Steps are taken to ensure users relying on LogRecordData#getBody() are not impacted, and continue to get a functional string representation even when the body is an AnyValue.

In this PR, getBody() continues to work by relying on AnyValue.asString() to provide a string encoding of complex types, but the new getAnyValueBody() is preferred. If I could do it all over again, I would have had the foresight to put Body in the api package from the start and avoided this problem.

trask

some (probably unhelpful) thoughts on naming:

AnyValue -> Value
AnyValueType -> ValueType
KeyAnyValue -> KeyValue or Entry
getAnyValueBody -> getBodyValue

sdk/testing/src/main/java/io/opentelemetry/sdk/testing/logs/TestLogRecordData.java

trask · 2024-07-22T19:54:52Z

api/all/src/main/java/io/opentelemetry/api/common/AnyValue.java

@@ -105,6 +108,9 @@ static AnyValue<List<KeyAnyValue>> of(Map<String, AnyValue<?>> value) {
   * Return a string encoding of this {@link AnyValue}. This is intended to be a fallback serialized
   * representation in case there is no suitable encoding that can utilize {@link #getType()} /
   * {@link #getValue()} to serialize specific types.
+   *
+   * <p>WARNING: No guarantees are made about the encoding of this string response. It MAY change in
+   * a future minor release. If you need a reliable string encoding, write your own serializer.
   */
  // TODO(jack-berg): Should this be a JSON encoding?


is there anything in spec about this? it seems like a lot of backends will get this format by default given the fallback implementation of getBody() and so it would be great if this format could be stable-ish

The closest we have is https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/common/attribute-type-mapping.md, which explains how to map data structures to AnyValue, but nothing about a string representation of AnyValue. If the spec did have something, I imagine it might either draw inspiration from or be defined as the standard protobuf JSON mapping.

I agree that a stable spec would be ideal. But I also think we can proceed without it. Ideally, exporters quickly update to be able to properly serialize AnyValue bodies. The goal of asString() is just to be a fallback allowing the information to be transmitted, but in a format which implies that you shouldn't depend on it. Seeing data in this format is a signal that something is suboptimal in the export pipeline, while still allowing things to work.

If you look at a test of AnyValue#asString, you see a format which is JSON-like, but definitely not actually JSON. The format is probably best described as an idiomatic implementation of Java toString(). It could easily be adapted to be actual valid JSON, but that might reduce the incentive for exporters to properly serialize.

jack-berg · 2024-07-23T21:03:59Z

some (probably unhelpful) thoughts on naming:
AnyValue -> Value
AnyValueType -> ValueType
KeyAnyValue -> KeyValue or Entry
getAnyValueBody -> getBodyValue

Thanks for the suggestions. I do like these. The one consideration I have is that the Any* prefix more strongly indicates a direct relationship with the protobuf AnyValue type. Is this valuable enough to justify the increased verbosity?

jack-berg · 2024-07-31T20:44:26Z

Ended up implementing trask's naming recommendations, which ballooned the size of the PR but is probably best in the long term.

breedx-splk · 2024-08-01T22:26:52Z

api/all/src/main/java/io/opentelemetry/api/logs/LogRecordBuilder.java

+   * Set the body string.
+   *
+   * <p>Shorthand for calling {@link #setBody(AnyValue)} with {@link AnyValue#of(String)}.
+   */
  LogRecordBuilder setBody(String body);


I think I'd rather see this string version have a default implementation that delegates to the AnyValue version, rather than the other way around. It seems safer and less potentially lossy that way.

Unfortunately we can't do this because. Because of our backwards compatibility guarantees, any new interface method we add has to have a default implementation. The default implementation should do the most sensible thing possible, which in this case is to call the existing setBody(String) method with a string encoding of the anyvalue.

Because of our backwards compatibility guarantees, any new interface method we add has to have a default implementation.

Note that these are the rules we've been abiding by, but we decided at the spec level that this type of compatibility isn't actually required. It allows alternative API implementations to continue to be able to compile when new API versions come out, but we decided its reasonable to say that alternative API implementations should have to stay up to date with API releases.

See this section of the spec: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/versioning-and-stability.md#extending-apisdk-abstractions

I'm inclined to continue with the practices we've been following unless doing so causes some egregious API user experience.

api/incubator/src/test/java/io/opentelemetry/api/incubator/logs/AnyValueTest.java

sdk/logs/src/main/java/io/opentelemetry/sdk/logs/SdkLogRecordBuilder.java

sdk/logs/src/test/java/io/opentelemetry/sdk/logs/AnyValueBodyTest.java

breedx-splk

This is great, and I'm happy to approve, just want us to give a consideration to which method has the default, otherwise looking good. Glad to see this moving forward.

breedx-splk

Thanks again!

…y-java into stabilize-log-any-value

jack-berg · 2024-08-27T19:50:10Z

@trask / @jkwatson any intent to spend additional time reviewing before I merge?

jkwatson · 2024-08-27T23:52:19Z

@trask / @jkwatson any intent to spend additional time reviewing before I merge?

I unfortunately won't have time. Make it so.

jack-berg · 2024-08-29T16:12:15Z

FYI, planning on merging later today.

jack-berg added 2 commits July 17, 2024 09:38

Promote LogRecordBuilder#setBody(AnyValue) to stable API

de530b8

Add stable SDK support for AnyValue body

885fc8b

jack-berg requested a review from a team July 17, 2024 18:34

Add test coverage

8e481f4

trask reviewed Jul 17, 2024

View reviewed changes

sdk/testing/src/main/java/io/opentelemetry/sdk/testing/logs/TestLogRecordData.java Outdated Show resolved Hide resolved

Improve comment wrapping

6cc771e

trask mentioned this pull request Jul 19, 2024

Save ILoggingEvent.getArgumentArray() arguments from Logback to support structured logging open-telemetry/opentelemetry-java-instrumentation#11861

Closed

trask reviewed Jul 22, 2024

View reviewed changes

TestLogRecordData handle Body.Type.EMPTY case

b875a73

Rename AnyValue to Value

6238736

breedx-splk reviewed Aug 1, 2024

View reviewed changes

api/incubator/src/test/java/io/opentelemetry/api/incubator/logs/AnyValueTest.java Outdated Show resolved Hide resolved

breedx-splk reviewed Aug 1, 2024

View reviewed changes

sdk/logs/src/main/java/io/opentelemetry/sdk/logs/SdkLogRecordBuilder.java Outdated Show resolved Hide resolved

breedx-splk reviewed Aug 1, 2024

View reviewed changes

sdk/logs/src/test/java/io/opentelemetry/sdk/logs/AnyValueBodyTest.java Outdated Show resolved Hide resolved

breedx-splk reviewed Aug 1, 2024

View reviewed changes

PR feedback

ae44c0e

jkwatson mentioned this pull request Aug 7, 2024

Java Logger API does not compliant with the OTel Logs Data Model #6626

Closed

yijiem mentioned this pull request Aug 7, 2024

C++ Logger API does not compliant with the OTel Logs Data Model open-telemetry/opentelemetry-cpp#3000

Open

laurit mentioned this pull request Aug 26, 2024

Structured logging in Slf4J not honoured in log entry Body open-telemetry/opentelemetry-java-instrumentation#12090

Open

breedx-splk approved these changes Aug 26, 2024

View reviewed changes

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

9764054

…y-java into stabilize-log-any-value

jack-berg merged commit 649f963 into open-telemetry:main Aug 30, 2024
18 checks passed

bogdandrutu mentioned this pull request Sep 5, 2024

Attributes "hell" open-telemetry/opentelemetry-specification#4201

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stabilize log any value #6591

Stabilize log any value #6591

jack-berg commented Jul 17, 2024

codecov bot commented Jul 17, 2024 •

edited

Loading

trask left a comment

jack-berg commented Jul 18, 2024

trask left a comment

trask Jul 22, 2024

jack-berg Jul 23, 2024

jack-berg commented Jul 23, 2024

jack-berg commented Jul 31, 2024

breedx-splk Aug 1, 2024

jack-berg Aug 6, 2024

jack-berg Aug 6, 2024

breedx-splk left a comment

breedx-splk left a comment

jack-berg commented Aug 27, 2024

jkwatson commented Aug 27, 2024

jack-berg commented Aug 29, 2024

Stabilize log any value #6591

Stabilize log any value #6591

Conversation

jack-berg commented Jul 17, 2024

codecov bot commented Jul 17, 2024 • edited Loading

Codecov Report

trask left a comment

Choose a reason for hiding this comment

jack-berg commented Jul 18, 2024

trask left a comment

Choose a reason for hiding this comment

trask Jul 22, 2024

Choose a reason for hiding this comment

jack-berg Jul 23, 2024

Choose a reason for hiding this comment

jack-berg commented Jul 23, 2024

jack-berg commented Jul 31, 2024

breedx-splk Aug 1, 2024

Choose a reason for hiding this comment

jack-berg Aug 6, 2024

Choose a reason for hiding this comment

jack-berg Aug 6, 2024

Choose a reason for hiding this comment

breedx-splk left a comment

Choose a reason for hiding this comment

breedx-splk left a comment

Choose a reason for hiding this comment

jack-berg commented Aug 27, 2024

jkwatson commented Aug 27, 2024

jack-berg commented Aug 29, 2024

codecov bot commented Jul 17, 2024 •

edited

Loading