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.

Sign up for GitHub

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

Jump to bottom

Clarify Body representation in Log Bridge API #3814

Closed
wants to merge 3 commits into from
Closed

Clarify Body representation in Log Bridge API #3814

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
70f8a2e
Clarify Body representation in Log Bridge API
pellared Jan 11, 2024
4c263b2
Merge branch 'main' into bridge-api-body
pellared Jan 11, 2024
58b3901
Merge branch 'main' into bridge-api-body
pellared Jan 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ release.

### Logs

- Clarify Body representation in Log Bridge API.
([#3814](https://github.com/open-telemetry/opentelemetry-specification/pull/3814))

### Resource

### OpenTelemetry Protocol
Expand Down
3 changes: 2 additions & 1 deletion specification/logs/bridge-api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).
djaglowski marked this conversation as resolved.
Show resolved Hide resolved
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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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

- [Attributes](./data-model.md#field-attributes)

All parameters are optional.
Expand Down
Loading