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

Attributes "hell" #4201

Open
bogdandrutu opened this issue Sep 4, 2024 · 10 comments
Open

Attributes "hell" #4201

bogdandrutu opened this issue Sep 4, 2024 · 10 comments
Labels
spec:logs Related to the specification/logs directory triage:deciding:community-feedback

Comments

@bogdandrutu
Copy link
Member

bogdandrutu commented Sep 4, 2024

What are you trying to achieve?

Share the same attributes across traces/metrics/logs. I do understand that the logs.attributes (this was a surprise that even attributes are different) and especially the body (not surprised about this) can be more generic than the traces and metrics, see https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#field-attributes and https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#field-body.

What did you expect to see?

Clear requirement in the bridge-api to support adding the "standard attributes" as well, since there are semantic conventions generate (example in go) that use the old style attributes.

Additional context.

Currently because of the logs data model saying that log record attributes are different than span/metrics attributes and that the bridge api reference to that https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/bridge-api.md#emit-a-logrecord

Java offers only support to add the "old style" attributes as attributes, see https://github.com/open-telemetry/opentelemetry-java/blob/main/api/all/src/main/java/io/opentelemetry/api/logs/LogRecordBuilder.java

Go offers only the more generic way, and I cannot share the attributes easily, see https://github.com/open-telemetry/opentelemetry-go/blob/main/log/record.go#L112

@bogdandrutu bogdandrutu added the spec:logs Related to the specification/logs directory label Sep 4, 2024
@cijothomas
Copy link
Member

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#field-attributes already states that, Log Attributes are a superset of Attributes allowed in Span,Metrics, and that was intentional.

Whether Event API should support the supertset or not: Its being discussed in #4199.

@bogdandrutu
Copy link
Member Author

bogdandrutu commented Sep 5, 2024

@cijothomas thanks for the link. I think the word "superset" does not imply that the "standard attributes" must be allowed in the LogRecord attributes or body. At least some SIGs did not understand that, see the golang example or the new Java Value where the "setBody" does not allow me to use the Attributes https://github.com/open-telemetry/opentelemetry-java/pull/6591/files#diff-7adba3bfe9b75261ce86c136c33a1e3f01a427c3ac7a378213567fc2d841f8aaR78

@pellared
Copy link
Member

pellared commented Sep 5, 2024

At least some SIGs did not understand that, see the golang example

We can always add a function converting a standard attribute to log attribute if it would occur to be needed.

EDIT: I created #4202

@jack-berg
Copy link
Member

the new open-telemetry/opentelemetry-java#6591 where the "setBody" does not allow me to use the Attributes https://github.com/open-telemetry/opentelemetry-java/pull/6591/files#diff-7adba3bfe9b75261ce86c136c33a1e3f01a427c3ac7a378213567fc2d841f8aaR78

The body is type AnyValue, not a Map<String, AnyValue> like attributes. The java setBody accepts a Value type (i.e. the java type for AnyValue) to reflect this. Allowing the body to have attribute key / value pairs added to it with something like addToBody(AttributeKey<T> key, T value) is saying two things: 1. I want the body to be a AnyValue type of key value list. 2. I want to add an entry to the key value list.

Given that the log bridge API is not intended to be user facing, API ergonomics are not a high priority. After all, how often do you need to reference standard attributes when writing a log appender?

Note that in java we did add good support for standard attributes to the Event API prototype: https://github.com/open-telemetry/opentelemetry-java/blob/e063b34a2ad1865b8e8bc0e8758e6c375efbf473/api/incubator/src/main/java/io/opentelemetry/api/incubator/events/EventBuilder.java#L79-L114

The difference being:

  1. The event API is user facing so ergonomics are important.
  2. We think event bodies will always be AnyValue of type key value list.

Note that this isn't necessarily the final say on the java log API. Its possible that we add syntactic sugar to make it easier to add standard attributes to the body like we've done with the Event API.

@jack-berg
Copy link
Member

Java is not aligned with the spec requirement that log attributes (i.e. not the body) are Map<String, AnyValue>. We could add support for that, but nobody has asked. If this is important to you, please open an issue and describe the use case.

@MrAlias
Copy link
Contributor

MrAlias commented Sep 5, 2024

Whether Event API should support the supertset or not: Its being discussed in #4199.

This implicitly conflicts with this:

Given that the log bridge API is not intended to be user facing, API ergonomics are not a high priority. [...] The event API is user facing so ergonomics are important.

If it follows that a public API should support the common attributes, then we need a way to convert to log attributes given the current design of the events API.

Otherwise, if we decide that the public Events API doesn't support common attributes, the argument that it's okay because it is only a bridge becomes invalidated.

@bogdandrutu
Copy link
Member Author

Java is not aligned with the spec requirement that log attributes (i.e. not the body) are Map<String, AnyValue>. We could add support for that, but nobody has asked. If this is important to you, please open an issue and describe the use case.

I think Java actually did the right thing keeping the log attributes as "standard attributes" because these are used to "identify a log entry". I actually want everyone for Attributes to use the "standard attributes" but for body we can have the "Any" value.

@bogdandrutu
Copy link
Member Author

@jack-berg depends how you read:

The log attribute model MUST support any type, a superset of standard Attribute, to preserve the semantics of structured attributes emitted by the applications. This field is optional.

From your comment:

Allowing the body to have attribute key / value pairs added to it with something like addToBody(AttributeKey key, T value) is saying two things: 1. I want the body to be a AnyValue type of key value list. 2. I want to add an entry to the key value list.

I want the KeyValue in java to allow me to use the "standard attributes" there. Why? Because per the comment that any is a superset I should be able to use the more restrictive set there.

@pellared
Copy link
Member

pellared commented Sep 5, 2024

Notes from SIG meeting #3849 (comment):

It may sense to have log attributes to be modeled differently than standard attributes as the purpose of Logs Bridge API is to support what existing structured logging libraries offer. Some applications use complex values for attributes and end-users would like to keep them.

@jack-berg
Copy link
Member

I want the KeyValue in java to allow me to use the "standard attributes" there. Why? Because per the comment that any is a superset I should be able to use the more restrictive set there.

This is totally possible in Java. See see my previous comment:

Given that the log bridge API is not intended to be user facing, API ergonomics are not a high priority. After all, how often do you need to reference standard attributes when writing a log appender?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:logs Related to the specification/logs directory triage:deciding:community-feedback
Projects
None yet
Development

No branches or pull requests

6 participants