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

Logs API to be user-facing #4225

Closed
wants to merge 24 commits into from
Closed

Conversation

pellared
Copy link
Member

@pellared pellared commented Sep 25, 2024

Follows open-telemetry/oteps#265

Key changes:

  1. Rename Logs Bridge API to Logs API.
  2. Give the languages a possibility to have separate APIs for Logs Bridge API and Logs API. This may be needed e.g. for Java, but also for other languages that do not find their current Logs Bridge API design well suited for "direct" usage.
  3. Call out that instrumentation libraries are supposed use the Logs API directly and applications can also use it directly if preferred.

Design decisions:

  1. Application developer initializes the log processing pipeline(s) by bootstrapping the logging provider using the Logs SDK. In most cases applications would use the same pipeline for logs and events. For edge cases application developers can build their own Logger Providers like [Spike] MultiLoggerProvider opentelemetry-go#5830. However, we will most likely need to add support for filtering and and log processing pipelines to the specification.
  2. Instrumentation libraries can use Logs API to emit events (log records with event.name attribute) so they do not depend (and not require depending) on any concrete logging library.
  3. Application developers use an log appender/bridge that connects their logging library with OTel via Logs API (or Logs Bridge API).
  4. Give freedom to application developers whether they prefer to use their logging library or Logs API (or even Events API) for emitting events (or any structured log records).
  5. This was foreseen as a possible option. From https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/bridge-api.md#artifact-naming:

    in the event OpenTelemetry were to add a user facing API, the Logs Bridge API would be a natural starting point. Therefore, Log Bridge API artifact, package, and class names MUST NOT include the terms "bridge", "appender", or any other qualifier that would prevent evolution into a user facing API.

  6. Currently an event is defined if a log record has an event.name attribute. The Events API and SDK is left in place as this would be too much in a single PR and we have no consensus nor compromise on the direction.

Related design doc by @tedsuo: https://docs.google.com/document/d/1iacum9SFGxV8VD0RKF6RPnIU75u2UnYnVcl_xjxvKmg/edit

@pellared pellared added spec:logs Related to the specification/logs directory enhancement New feature or request labels Sep 25, 2024
Copy link
Contributor

@MSNev MSNev left a comment

Choose a reason for hiding this comment

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

Whatever form the new Logs API takes will be a merging (not a removal) of the existing Event API.

If there is any "Rename" (IMHO) it would be that the Event API is renamed and have functionality added to it.

ALthough, I still prefer having these as separate API's where the SDK's can implement more than one.

Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
specification/logs/api.md Outdated Show resolved Hide resolved
specification/logs/api.md Outdated Show resolved Hide resolved
specification/logs/api.md Outdated Show resolved Hide resolved
pellared and others added 2 commits September 26, 2024 06:26
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Co-authored-by: Reiley Yang <reyang@microsoft.com>
@pellared
Copy link
Member Author

pellared commented Sep 26, 2024

I still prefer having these as separate API's where the SDK's can implement more than one.

@MSNev, I did my best to make it acceptable (key change number 2) and codified it as:

Languages MAY provide a separate Logs Bridge API if they need different ergonomics for consumers that are building log appenders/bridges.

@pellared pellared requested a review from MSNev September 26, 2024 18:25
specification/logs/README.md Outdated Show resolved Hide resolved
specification/logs/README.md Show resolved Hide resolved
specification/logs/api.md Outdated Show resolved Hide resolved
pellared and others added 2 commits September 27, 2024 06:34
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
@pellared pellared requested a review from trask September 27, 2024 06:44
pellared and others added 2 commits September 27, 2024 18:29
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Copy link
Contributor

@MSNev MSNev left a comment

Choose a reason for hiding this comment

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

To support it we do not need to have a dedicated API. There is/was also a debate whether event name should be as a LogRecord field instead of an attribute, but this is out of scope for this PR.

YES WE DO!

As per the OTep

API

OpenTelemetry SHOULD provide a (user-facing) Logs API that includes the capability to emit OpenTelemetry Events.

And this "capability" SHOULD NOT just be "just include the event.name attribute" into a Log record.

Again as per the OTep

What are OpenTelemetry Events?

They are a core concept in OpenTelemetry Semantic Conventions.

And therefore as a CORE concept it SHOULD have a CORE API (ie. emitEvent) so that any and all semantic conventions current or future can be implemented and enforced / applied.

Features for the [Events API](specification/logs/event-api.md) and the [Events SDK](specification/logs/event-sdk.md).
Disclaimer: Events are currently in Development status - work in progress.

| Feature | Optional | Go | Java | JS | Python | Ruby | Erlang | PHP | Rust | C++ | .NET | Swift |
Copy link
Contributor

Choose a reason for hiding this comment

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

This table is (was) incomplete, several languages already have this capability and users of this API.

JS, Java (Android) and recently Python

CHANGELOG.md Outdated
@@ -31,6 +31,8 @@ release.
([#4203](https://github.com/open-telemetry/opentelemetry-specification/pull/4203))
- Make all fields as identifying for Logger. Previously attributes were omitted from being identifying.
([#4161](https://github.com/open-telemetry/opentelemetry-specification/pull/4161))
- Remove Events API and SDK. Rename Logs Bridge API to Logs API and do not discourage from direct usage.
Copy link
Contributor

Choose a reason for hiding this comment

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

Merge Events API and SDK

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 decided to simply bring back the Events API and SDK. We can address it later as a separate PR.

@pellared
Copy link
Member Author

pellared commented Sep 27, 2024

SIG meeting notes:
There are use cases where languages would want to have dedicated API just for emitting events. Therefore, the proposal is to bring back the Events API. If a language provides Events API then the Logs SDK SHOULD implement it. Some languages (e.g. Go) do not want to provide a separate Events API therefore Logs API can be used for emitting events. 👍, 👎, and comments are welcome before I make changes in the PR 😉

I guess that they may be two category of languages:
A) languages that provide Logs Bridge API and Events API
B) languages that provide Logs API

@trask
Copy link
Member

trask commented Sep 27, 2024

Therefore, the plan is to bring back the Events API

this would contradict the OTEP (https://github.com/open-telemetry/oteps/blob/main/text/0265-event-vision.md#api):

OpenTelemetry SHOULD provide a (user-facing) Logs API that includes the capability to emit OpenTelemetry Events.

@@ -31,6 +31,8 @@ release.
([#4203](https://github.com/open-telemetry/opentelemetry-specification/pull/4203))
- Make all fields as identifying for Logger. Previously attributes were omitted from being identifying.
([#4161](https://github.com/open-telemetry/opentelemetry-specification/pull/4161))
- Make Logs Bridge API as the Logs API and do not discourage from direct usage.
Copy link
Contributor

Choose a reason for hiding this comment

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

No, this PR should be about either

  • Merging the entire Logs Bridge API AND the Events API (as is) and marking ALL as in-progress
  • Creating a new EMPTY Logs API stating with comments in the Events and Bridge API stating that the new Logs API will encompass ALL of the functionality from the currently represented in the current API's (with the exact details to be worked on)

Copy link
Member

Choose a reason for hiding this comment

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

creating a new logs/api.md and leaving both the existing (stable) logs/bridge-api.md and (experimental) logs/event-api.md alone sounds like a good path forward.

the logs/api.md page can explain that one of its goals is to replace the Event API and possibly also the Bridge API.

Copy link
Member Author

@pellared pellared Sep 30, 2024

Choose a reason for hiding this comment

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

I would prefer not creating yet another API. The specification was foreseeing that Logs Bridge API may be evolved into Logs API. I thing that having them separated in the specification will make it harder to follow and maintain. This is the main motivation behind this PR.

I strongly prefer having a separate, alternate PR instead of changing this one. @MSNev, can you create a new PR?

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer not creating yet another API. The specification was foreseeing that Logs Bridge API may be evolved into Logs API. I thing that having them separated in the specification will make it harder to follow and maintain. This is the main motivation behind this PR.

makes sense, do you think we need to change the page to Mixed stability and call out anything?

Copy link
Member Author

@pellared pellared Sep 30, 2024

Choose a reason for hiding this comment

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

If there are good reasons to have the changes as Development then yes. I did my best to limit the scope so that it may be seen as acceptable in the Stable portion of the specification.

Copy link
Member Author

@pellared pellared Sep 30, 2024

Choose a reason for hiding this comment

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

Maybe I should mark Convenience section as Development as this is a new "functional' addition to logs. The rest is more about the purpose of the API.

Copy link
Member

Choose a reason for hiding this comment

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

I've added a topic to tomorrow's Spec SIG agenda:

  • Any gotchas to renaming the “Logs Bridge API” to “Logs API” inside of a stable document?

OpenTelemetry to ship a feature-rich logging library.

- OpenTelemetry defines an [SDK](./sdk.md) implementation of the [Bridge API](./bridge-api.md),
- OpenTelemetry defines a Logs API for [emitting LogRecords](./api.md#emit-a-logrecord).
Copy link
Contributor

Choose a reason for hiding this comment

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

This NEEDS to include comments about also emitting events as that is the intention

Copy link
Member Author

Choose a reason for hiding this comment

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

It is described in API and Data Model. Do we need to repeat it? Can you suggest a change?

@@ -444,10 +445,9 @@ standard output.

## Specifications

* [Logs Bridge API](./bridge-api.md)
* [Logs API](./api.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

How about having links to the old API's stating that the this new API will encompass the existing functionality.

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 do not want to create yet another API.

specific guarantees and safeties.

**LoggerProvider** - all methods are safe to be called concurrently.

**Logger** - all methods are safe to be called concurrently.

## Artifact Naming
## Convenience
Copy link
Contributor

Choose a reason for hiding this comment

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

This is PART of the issue with this process.

Events are MORE than just a Convenience!

I would be fine with wording along the lines of

For languages that do not need or want to provide any OpenTelemetry Event support because they have their own native or common libraries, then they MAY choose to not implement the emitEvent functionality and support.

Copy link
Member Author

Choose a reason for hiding this comment

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

Events are MORE than just a Convenience

I find this as an opinionated statement based on the current definition of Opentelemetry Events

Copy link
Contributor

Choose a reason for hiding this comment

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

I find this as an opinionated statement based on the current definition of Opentelemetry Events

And THAT is the crux of the disagreement

You (and several others) DON'T believe and either DON'T understand or WANT to understand that "Events" are NOT JUST Logs, so your Opinionated view / statement is to remove something that you DON'T understand.

As stated in the Oel (this portion I agree with)

OpenTelemetry Events are a type of OpenTelemetry Log that requires an event name and follows a specific structure implied by that event name.

They are a TYPE of Log, it does not state that ALL Logs are Events and it does not state the there is ONLY 1 TYPE of Log (Events). Calling Every LOG and Event destroys the Semantic meaning of what is an Event.

That does not mean that there isn't room to "define" the appropriate Semantics to keep Both, but that is the discussions and part of the agreements that we need to make BEFORE we can conclude this PR (as I've mentioned eariler)

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we call this section "Optional methods" so it works for everyone?

Copy link
Member Author

@pellared pellared Sep 30, 2024

Choose a reason for hiding this comment

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

I would rather remove the whole section.

I think it is needed not only for sake of events but e.g. Java may want to have convenient methods like Info, Warn, Error. I find a EmitEvent as a similar convience method as it is not necessary for the users that want to emit events.

Calling Every LOG and Event destroys the Semantic meaning of what is an Event.

Nobody does it. Currently, just call every log with event.name attribute as an Event. For the SDK/Data Model/Collector perspective it does not matter how it was created. This is currently the way an Event is defined and I am not making any change to it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another problem, I see in defining Emit an Event method (in scope of this PR) is that there would be an ambiguity what should happen if a user adds an additional event.name attribute. Should the name parameter or event.name attribute have precedence?

PS. The same problem currently exists in https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/event-api.md#emit-event

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 #4233

And also committed cc0f262 to avoid this issue in scope of this PR


From OpenTelemetry's perspective LogRecords and Events are both represented
using the same [data model](./data-model.md). An Event is a specialized type
of LogRecord, not a separate concept.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the not a separate concept or rephase that OpenTelemetry encapsulates and transmits as a LogRecord

From OpenTelemetry's perspective LogRecords and Events are both represented
using the same data model. As such an Event is sent as a LogRecord
with an identifying event.name attribute.

Copy link
Member Author

Choose a reason for hiding this comment

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

No. This is an extract from Events API. I have not changed a single word. Out of scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the framing of the Event API as a separate entity and therefore the creation and sending of the resulting Event as a LogRecord (and not a separate Event concept) it make sense there. If does NOT make sense as part of folding into a SINGLE Api which emits both because now they ARE 2 separate concepts that are emitted from the same API.

Copy link
Member Author

@pellared pellared Sep 30, 2024

Choose a reason for hiding this comment

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

they ARE 2 separate concepts

(As of today's understanding of the specification) They are not. Event is a specialization of a LogRecord. I never have seen in spec that Events and Logs are separate signals (this would mean for me that these are separate concepts).


Events are OpenTelemetry's standardized semantic formatting for LogRecords.
Beyond the structure provided by the LogRecord data model, it is helpful for
logs to have a common format within that structure. When OpenTelemetry
Copy link
Contributor

Choose a reason for hiding this comment

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

And this is the crux of the problem

When OpenTelemetry instrumentation emits logs, those logs SHOULD be formatted as Events.

While Events are represented as LogRecords not all Logs are Events

Copy link
Member Author

@pellared pellared Sep 30, 2024

Choose a reason for hiding this comment

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

I do not follow what this comment is about

Copy link
Contributor

Choose a reason for hiding this comment

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

Again the crux of the issue that needs to be discussed during the meeting synchronously and not asynchronously where everyone only has a narrow view of these things.

Copy link
Contributor

Choose a reason for hiding this comment

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

See not just my comments but also @jkwatson open-telemetry/oteps#265 (comment) in the OTep.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to be a copy-paste from the existing

Events are OpenTelemetry's standardized semantic formatting for LogRecords.
Beyond the structure provided by the LogRecord data model, it is helpful for
logs to have a common format within that structure. When OpenTelemetry
instrumentation emits logs, those logs SHOULD be formatted as Events. All
semantic conventions defined for logs MUST be formatted as Events.

but I wonder if we should allow instrumentations to emit logs or events now that logging api is going to be user-facing.

Left another comment on it - https://github.com/open-telemetry/opentelemetry-specification/pull/4225/files#r1781573289

Copy link
Contributor

@MSNev MSNev left a comment

Choose a reason for hiding this comment

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

Still needs work

@reyang
Copy link
Member

reyang commented Sep 30, 2024

@MSNev, @lmolkova, @trask, @reyang, @cijothomas: PTAL

Let me know if you are OK to "undraft" the PR (mark is as "Ready for review").

+1 on removing the "Draft" status.

@trask
Copy link
Member

trask commented Sep 30, 2024

@MSNev, @lmolkova, @trask, @reyang, @cijothomas: PTAL
Let me know if you are OK to "undraft" the PR (mark is as "Ready for review").

+1 on removing the "Draft" status.

👍 I think it will be good to widen the circle of reviewers, and I plan to raise this PR in the Spec meeting tomorrow, so will be good if more people take a look at it before then

@MSNev
Copy link
Contributor

MSNev commented Sep 30, 2024

@MSNev, @lmolkova, @trask, @reyang, @cijothomas: PTAL
Let me know if you are OK to "undraft" the PR (mark is as "Ready for review").

+1 on removing the "Draft" status.

👍 I think it will be good to widen the circle of reviewers, and I plan to raise this PR in the Spec meeting tomorrow, so will be good if more people take a look at it before then

Why the push to RAM this change through?

We have not answered some of the basic fundamental questions and not had the first official meeting to even start to address some the basic mis-understandings and concepts that you are preparing to throw out.

There is LOTS of common ground that we can build one and the SHOULD be documented, but this PR (in it's current state) is not doing that

OpenTelemetry log data model. See also: [Logs Bridge API](#logs-bridge-api)

It is provided for (instrumentation) libraries to emit
[OpenTelemetry Events](data-model.md#events).
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe by making Otel Logs API user-facing, we should not limit instrumentation to events. It should be fine to emit nameless log records.

So maybe

Suggested change
[OpenTelemetry Events](data-model.md#events).
logs.

?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe by making Otel Logs API user-facing, we should not limit instrumentation to events. It should be fine to emit nameless log records.

So maybe

?

ABSOLUTELY!

And whether that are called "nameless" or there is some other "name" (not event.name) that describes that type (options discussed include log.name, log.record.name maybe even a new log.type (But the enumeration of this is extremely problematic as what "level" do you define)) of Log entry is specifically being written is just part of the unanswered questions.

Copy link
Member Author

@pellared pellared Sep 30, 2024

Choose a reason for hiding this comment

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

It is about OTel instrumentation libraries (not any "instrumentation").

When OpenTelemetry instrumentation emits logs, those logs SHOULD be formatted as Events.

from https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/event-api.md#event-data-model

as well as https://github.com/open-telemetry/oteps/blob/58d6420237091bcfb739f329e09adaa7ec03f702/text/0265-event-vision.md?plain=1#L29-L32

The next paragraph is about using the API to emit any logs.

Copy link
Contributor

Choose a reason for hiding this comment

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

And both of those links talked about them be "represented" by the same Data Model, or as a "type" of Log, it is NOT stating that this makes them the SAME thing (beyond the transport / representation), they still HAVE different Semantics.
Circling back to the OTep this is the HOW are they SENT not WHAT are they.

What is an Event https://github.com/open-telemetry/semantic-conventions/blob/main/docs/general/events.md
What is a Log https://github.com/open-telemetry/semantic-conventions/blob/main/docs/general/logs.md

Nowhere is it saying that a Log is an Event

Copy link
Contributor

Choose a reason for hiding this comment

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

And as an example of additional cases where I'm saying that we are talking past each other on the possible solutions,
Please go and see my comment from Sept 9th IN THE OTEP
open-telemetry/oteps#265 (comment), in the design section and the "Combining API's" (the exact thing you are trying to define here)

Nothing over there was "simply" ranting, it was about trying to inform and provide workable solutions that will work for ALL of US and the community.

Copy link
Member Author

@pellared pellared Sep 30, 2024

Choose a reason for hiding this comment

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

there could also be things I just want to let my users know of via logs. E.g. certain configuration was picked, connection has dropped, etc. I could use 3rd party logging library, but why if I can use OTel logging API without adding a new dependency

How would the instrumentation library report a problem if the OTel Logs Provider is not configured?

Some OTel libraries have already some ways to report problems. E.g. in Go we have https://pkg.go.dev/go.opentelemetry.io/otel#SetErrorHandler.

I am not against the idea, but I think it would increase the impact (and scope) of this PR. How about a separate issue after this one is (hopefully) merged?

Copy link
Contributor

@lmolkova lmolkova Sep 30, 2024

Choose a reason for hiding this comment

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

How would the instrumentation library report a problem if the OTel Logs Provider is not configured?

it won't - I mean someone could configure default provider (stdout), but in general case if you use 3rd party logging library, it's not a responsibility of something that emits logs to notify you that your logging provider is not configured.

We can borrow some ideas from logging libs. E.g. SLF4J notifies you if provider is not configured with something like this in stderr:

image

I agree that it's not a viable choice for instrumentation today to rely on OTel logs being configured as a main source of logging, but 3 years from now it could be a nice option to report all telemetry with otel instead of taking dependency on something else.

And we're not restricting anything - we're adding more options.

In any case, I'd like to have this discussion tomorrow on the spec call, but if you feel strongly about making progress on this PR without addressing it, I'm fine doing it as a follow up.

Copy link
Member Author

@pellared pellared Sep 30, 2024

Choose a reason for hiding this comment

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

I created #4234 please feel free to update the description to make it more complete

Copy link
Member

Choose a reason for hiding this comment

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

Imagine I'm instrumenting kafka:

  • there could be some events I'd emit - e.g. cluster leader is unreachable - which are documented in the semconv

  • there could also be things I just want to let my users know of via logs. E.g. certain configuration was picked, connection has dropped, etc. I could use 3rd party logging library, but why if I can use OTel logging API without adding a new dependency?

The library logs are emitted with is not relevant to users - they'd get the same logs if lib used 3rd party logger or OTel API. So I don't see why we should limit instrumentations from using OTel API to emit undocumented regular (but structured) logs.

I think this is a good example. You can emit an untyped event (which is a nameless logrecord), and it would bypass any event pipelines set up, but it would still give you a single api surface at the low level.

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW @pellared my main focus with this is native instrumentations where now I need to pick one API for logging and use OTel for everything else. I.e. I don't mean instrumentation logs, I mean the underlying library logs.

@trask
Copy link
Member

trask commented Sep 30, 2024

Why the push to RAM this change through?

No one is ramming anything through, we're just adding it to the Spec meeting agenda because there are some tricky questions around stability that could benefit from discussion with the @open-telemetry/technical-committee

Comment on lines +34 to +35
The Events API was designed to allow shared libraries to emit
[events](data-model.md#events) without needing to depend on a third party logger.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this part is changing now.

Maybe

Suggested change
The Events API was designed to allow shared libraries to emit
[events](data-model.md#events) without needing to depend on a third party logger.
The Events API was designed to allow shared libraries and applications to emit named and structured logs with consistent semantics.

?

Or maybe just remove this sentence?

Copy link
Member Author

Choose a reason for hiding this comment

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

LGTM. We can the apply your suggestion if there are no counter arguments.

Copy link
Contributor

Choose a reason for hiding this comment

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

It was designed to emit named and structured EVENTS that happen to be sent / represented as a LogRecord, they are still EVENTS and not JUST A LOG!

Copy link
Contributor

Choose a reason for hiding this comment

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

@MSNev can you make a suggestion that would work for you? The original sentence is no longer applicable, what would be a good replacement?

Copy link
Member Author

Choose a reason for hiding this comment

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

But they are still logs 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it no longer applicable, the original sentence is still valid. If the Events API is being merged into a new common Log API then nothing has changed.

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 agree with @MSNev that the current sentence is still applicable/valid. @lmolkova, how about resolving?

Copy link
Contributor

@lmolkova lmolkova Sep 30, 2024

Choose a reason for hiding this comment

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

I think it's not relevant since with this PR we will have user-facing logging API.

I.e. the second part of this sentence no longer makes sense: "The Events API was designed to allow shared libraries to emit events without needing to depend on a third party logger"

Copy link
Member Author

@pellared pellared Sep 30, 2024

Choose a reason for hiding this comment

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

Then maybe

Suggested change
The Events API was designed to allow shared libraries to emit
[events](data-model.md#events) without needing to depend on a third party logger.
The Events API was designed to allow shared libraries to emit
[events](data-model.md#events).

Copy link
Contributor

@lmolkova lmolkova Sep 30, 2024

Choose a reason for hiding this comment

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

this sounds precise and resolves my concern.

I feel this statement ("X API was designed to .. emit X") does not carry any useful information though😄 but if you believe that it does, I don't have any objections against it

@lmolkova
Copy link
Contributor

We have not answered some of the basic fundamental questions and not had the first official meeting to even start to address some the basic mis-understandings and concepts that you are preparing to throw out.

The way this PR is written now it does not seem to change anything around event definition. The wording around events is copy-pasted with no change. So the only fundamental questions it raises now are around log API and it's stability. Let's try to review it from this perspective and if there are any concerns around existing wording in the spec, let's create issues and proposals separately from this PR.

@MSNev
Copy link
Contributor

MSNev commented Sep 30, 2024

We have not answered some of the basic fundamental questions and not had the first official meeting to even start to address some the basic mis-understandings and concepts that you are preparing to throw out.

The way this PR is written now it does not seem to change anything around event definition.

Please see my review comments.

The wording around events is copy-pasted with no change. So the only fundamental questions it raises now are around log API and it's stability. Let's try to review it from this perspective and if there are any concerns around existing wording in the spec, let's create issues and proposals separately from this PR.

This is more than just Copying and pasting, "moving" changes context, AND I provided my review comments on the basis of moving forward and ensuring that the reasoning originally provided for the wording and definitions is not lost in the process.

@lmolkova
Copy link
Contributor

This is more than just Copying and pasting, "moving" changes context, AND I provided my review comments on the basis of moving forward and ensuring that the reasoning originally provided for the wording and definitions is not lost in the process.

To clarify, you don't agree with moving Event Data Model section (as is) from event-api.md file to data-model.md file?

@@ -466,6 +467,28 @@ of the record.
If included, they MUST follow the OpenTelemetry
[semantic conventions for exception-related attributes](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/exceptions/exceptions-logs.md).

### Events
Copy link
Member

Choose a reason for hiding this comment

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

should this section be marked with stability Development?

## Artifact Naming
## Convenience

Languages MAY provide additional ergonomics and convinence APIs. For instance,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Languages MAY provide additional ergonomics and convinence APIs. For instance,
Languages MAY provide additional ergonomics and convenience APIs. For instance,

specification/logs/event-api.md Outdated Show resolved Hide resolved
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Copy link

github-actions bot commented Oct 8, 2024

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 Oct 8, 2024
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request spec:logs Related to the specification/logs directory Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants