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

Clarify Body representation in Log Bridge API #3814

Closed
wants to merge 3 commits into from

Conversation

pellared
Copy link
Member

@pellared pellared commented Jan 11, 2024

Superseded by #3827

Changes

Clarify Body representation in Log Bridge API as to make interpretation of the specification easier.

Why

I noticed that the Logs Data Model defines Body to be any (not a string).

However, the spec in the same section later says:

First-party Applications SHOULD use a string message.

The spec here says that the application is First-Party App if it is using Bridge API. Therefore, the spec recommends passing Body via Bridge API as string.

Here is how the languages are currently representing Body in their Logs Bridge APIs:

  • Java (stable logs): Defines Body as string.
  • .NET (stable logs): Defines Body as string.
  • C++ (stable logs): Defines Body as attribute value (however a map as value is not supported).
  • Python (experimental logs): Defines Body as Any (in Python sense which means it can be anything).

However, I think that the Bridge API can still accept a structured body (any). This would give more flexibility to the Bridge API consumer and also can be helpful when if we the user would like to pass events (structured logs).

Slack thread: https://cloud-native.slack.com/archives/C01N7PP1THC/p1704907537212259

Related to open-telemetry/opentelemetry-go#4809

@pellared pellared marked this pull request as ready for review January 11, 2024 09:58
@pellared pellared requested review from a team January 11, 2024 09:58
@@ -120,7 +120,8 @@ The API MUST accept the following parameters:
behavior.
- [Severity Number](./data-model.md#field-severitynumber)
- [Severity Text](./data-model.md#field-severitytext)
- [Body](./data-model.md#field-body)
- [Body](./data-model.md#field-body). The API MUST support passing Body as `string`.
The API MAY support passing Body as a type representing [`any`](./data-model.md#type-any).
Copy link
Member

Choose a reason for hiding this comment

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

This is a Stable document and we are not allowed breaking changes.

I think is a breaking change. It is not a choice to support any type. The reference to (also Stable) log data model clearly defines the type of Body and it is any, not a string.

I would like to hear arguments in favour of somehow considering this as an allowed change. Blocking until then.

Copy link
Member Author

@pellared pellared Jan 11, 2024

Choose a reason for hiding this comment

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

There is no place in the specification that enforces the Bridge API to support the any type.

From the PR description:

I noticed that the Logs Data Model defines Body to be any (not a string).

However, the spec in the same section later says:

First-party Applications SHOULD use a string message.

The spec here says that the application is First-Party App if it is using Bridge API. Therefore, the spec recommends passing Body via Bridge API as string.

Copy link
Member

Choose a reason for hiding this comment

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

There is no place in the specification that enforces the Bridge API to support the any type.

True, not explicitly. But implicitly, by inference the type definition in log data model of course applies (for all fields that link to it, including Body), otherwise it is impossible to understand what the Emit method is supposed to do. The reason Bridge API does not say so is because it would be just a duplication of the same information.

Notice how "First-party Application" does not directly call Bridge API. So, strictly speaking what "First-party Application" is required to do is only a subset of what Bridge API needs to support, it does not need to be an exact match. Also the README document you refer to is non-normative, its purpose is to explain, not to serve as precise specification.

So far I am not convinced the existing vague wording about "First-party Application" absolves Bridge API from supporting any type for Body.

Copy link
Member Author

@pellared pellared Jan 11, 2024

Choose a reason for hiding this comment

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

So far when working on OTel Go Logs Bridge API prototype I have not found any popular Go logging library where I would want to put anything other than a string in Body.

This is also supported by:https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model-appendix.md which always suggests putting the log record text message as Body.

Copy link
Member Author

@pellared pellared Jan 11, 2024

Choose a reason for hiding this comment

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

First-party Applications SHOULD use a string message.

Why the Data Model even references "First-party Application" and uses normative language? It suggests that the API or SDK author should do something with information. How would someone reviewing the API+SDK conforms with this statement?

We could change the statement to:

Suggested change
The API MAY support passing Body as a type representing [`any`](./data-model.md#type-any).
The API MUST support passing Body as a type representing [`any`](./data-model.md#type-any).

But I am not sure if others would agree with such interpretation. I would rather do it as a separate PR as I think it is more controversial:

  • Based on how Java and .NET implemented Bridge API and that so far I see no need for it to support most Go logging libraries
  • Right now there is no Bridge API that accepts Body as a type representing any defined in Logs Data Model.

Copy link
Member

@lalitb lalitb Jan 11, 2024

Choose a reason for hiding this comment

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

Right now there is no Bridge API that accepts Body as a type representing any defined in Logs Data Model.

As I understand, otel-rust accept Body as a type representing any as per the Log Data Model. However the signal is not yet stable and can be changed. The otel-cpp, as you mentioned, uses the attribute's value type to represent the Body, and the signal is already stable.

Copy link
Member Author

Choose a reason for hiding this comment

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

I created #3827

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

@pellared Did you dismiss my request to change? Please don't do that. I am going to block again until we have a resolution.

@tigrannajaryan
Copy link
Member

I would like other @open-telemetry/specs-approvers to assess this change and help interpret our spec stability guarantees. Do you think this change is inline with our "Stable" document promises?

@pellared
Copy link
Member Author

pellared commented Jan 12, 2024

@pellared Did you dismiss my request to change? Please don't do that. I am going to block again until we have a resolution.

Nope. I just did the following after adding some comments:

pellared requested a review from tigrannajaryan yesterday

The "requesting changes" is still there only the "icon" changes (the UI is misleading but the PR is still blocked from being merged).

I am not in a rush and I do not insist on any concrete way to solve it. My main intention is to clarify the specification as I find currently as misleading.

We can e.g. also consider removing the following fragment which I find useless and only brings confusion:

First-party Applications SHOULD use a string message.
However, a structured body SHOULD be used to preserve the semantics of structured logs emitted by Third-party
Applications.
Can vary for each occurrence of the event coming from the same
source.

I plan to join next Specification SIG meeting to discuss it.

I appreciate your feedback 👍

EDIT: I created #3827

@pellared
Copy link
Member Author

Superseded by #3827

@pellared pellared marked this pull request as draft January 16, 2024 18:24
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jan 24, 2024
@pellared pellared closed this Jan 25, 2024
tigrannajaryan pushed a commit that referenced this pull request Feb 6, 2024
Related to
#3814

## Changes

Remove confusing description from Body field in Logs Data Model.

The description makes the specification not clear as the language
authors may understand that the Bridge API must only accept Body as
`string`.

Additionally, I do not think that the following recommendation is
actually a good one:

> First-party Applications SHOULD use a string message

---------

Co-authored-by: jack-berg <[email protected]>
Co-authored-by: Trask Stalnaker <[email protected]>
Co-authored-by: Cijo Thomas <[email protected]>
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
…-telemetry#3827)

Related to
open-telemetry#3814

## Changes

Remove confusing description from Body field in Logs Data Model.

The description makes the specification not clear as the language
authors may understand that the Bridge API must only accept Body as
`string`.

Additionally, I do not think that the following recommendation is
actually a good one:

> First-party Applications SHOULD use a string message

---------

Co-authored-by: jack-berg <[email protected]>
Co-authored-by: Trask Stalnaker <[email protected]>
Co-authored-by: Cijo Thomas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants