-
Notifications
You must be signed in to change notification settings - Fork 855
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
Stabilize log any value #6591
Stabilize log any value #6591
Changes from 3 commits
de530b8
885fc8b
8e481f4
6cc771e
b875a73
6238736
ae44c0e
9764054
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
|
||
package io.opentelemetry.api.logs; | ||
|
||
import io.opentelemetry.api.common.AnyValue; | ||
import io.opentelemetry.api.common.AttributeKey; | ||
import io.opentelemetry.api.common.Attributes; | ||
import io.opentelemetry.context.Context; | ||
|
@@ -66,9 +67,19 @@ | |
/** Set the severity text. */ | ||
LogRecordBuilder setSeverityText(String severityText); | ||
|
||
/** Set the body string. */ | ||
/** | ||
* Set the body string. | ||
* | ||
* <p>Shorthand for calling {@link #setBody(AnyValue)} with {@link AnyValue#of(String)}. | ||
*/ | ||
LogRecordBuilder setBody(String body); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
|
||
/** Set the body {@link AnyValue}. */ | ||
default LogRecordBuilder setBody(AnyValue<?> body) { | ||
setBody(body.asString()); | ||
return this; | ||
} | ||
|
||
/** | ||
* Sets attributes. If the {@link LogRecordBuilder} previously contained a mapping for any of the | ||
* keys, the old values are replaced by the specified values. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-ishThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.