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

Make sure it is very clear we are not building a Logging API #2966

Conversation

tigrannajaryan
Copy link
Member

We are only building a logging "backend" which is supposed to be used via appenders by existing logging libraries. Additionally, we may provide helpers to build "events" that make easier to build LogRecords with specific semantic convention. Such helpers will also call the logging "backend" APIs.

We do not intend to have user-facing logging API in the spec.

Resolves #2936

We are only building a logging "backend" which is supposed to be used via appenders by existing logging libraries. Additionally, we may provide helpers to build "events" that make easier to build LogRecords with specific semantic convention. Such helpers will also call the logging "backend" APIs.

We do not intend to have user-facing logging API in the spec.

Resolves open-telemetry#2936
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Alternatively we could start calling this SPI instead of API, since application is not supposed to call it. Here's a nice definition from SO (or another one)

API stands for Application Programming Interface, where API is a means for accessing a service / function provided by some kind of software or a platform.

SPI stands for Service Provider Interface, where SPI is way to inject, extend or alter the behavior for software or a platform.

@jack-berg
Copy link
Member

Alternatively we could start calling this SPI instead of API, since application is not supposed to call it.

I like that. Though currently the event API is expected to be used by the application, and has a dependency on the log API. So the log API isn't supposed to be used by application code except in the special case where Logger is passed as an argument to the event API. Do you think this dependency changes anything around calling it an SPI vs API?

@tigrannajaryan
Copy link
Member Author

... currently the event API is expected to be used by the application, and has a dependency on the log API. So the log API isn't supposed to be used by application code except in the special case where Logger is passed as an argument to the event API.

Because of this the Logger API must be placed in the API package. If we call it an SPI can it possible cause confusion?

@arminru arminru added spec:logs Related to the specification/logs directory area:api Cross language API specification issue editorial Editorial changes only (typos, changelog, ...). No content-related changes of any kind. labels Nov 21, 2022
specification/logs/api.md Show resolved Hide resolved
@trask
Copy link
Member

trask commented Nov 22, 2022

... currently the event API is expected to be used by the application, and has a dependency on the log API. So the log API isn't supposed to be used by application code except in the special case where Logger is passed as an argument to the event API.

Because of this the Logger API must be placed in the API package. If we call it an SPI can it possible cause confusion?

Can we break the dependency between Event API and Log API by having the Log SDK implement the Event API?

@github-actions
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 Nov 30, 2022
@tigrannajaryan
Copy link
Member Author

Can we break the dependency between Event API and Log API by having the Log SDK implement the Event API?

I don't understand what this means.

To reiterate one more time: the code that calls an Event API typically lives somewhere in instrumentation libraries. Instrumentation libraries must depend only on API package and cannot call methods from Log SDK directly.

@tigrannajaryan
Copy link
Member Author

I am going to merge this since I believe the warning added by this PR is important. Further refinements and a discussion to call it an SPI can be done on a separate PR.

@tigrannajaryan tigrannajaryan merged commit 0a4c665 into open-telemetry:main Nov 30, 2022
@tigrannajaryan tigrannajaryan deleted the feature/tigran/clarify-no-log-frontend branch November 30, 2022 14:26
@trask
Copy link
Member

trask commented Nov 30, 2022

Can we break the dependency between Event API and Log API by having the Log SDK implement the Event API?

I don't understand what this means.

basically, I mean:

User code -> Event API -> Log SDK

instead of:

User code -> Event API -> Log API -> Log SDK

@tigrannajaryan
Copy link
Member Author

basically, I mean:

User code -> Event API -> Log SDK

@trask I think the way we envision the Event API to look like and be used this is impossible. The expected usage in instrumentation libraries is the following:

loggerProvider := GetLoggerProvider()
logger := loggerProvider.GetLogger("mylibrary")
evtLogger := GetEventLogger(logger, "mydomain")
data := ...
evtLogger.Emit("myevent", data)

The GetLoggerProvider, GetLogger and GetEventLogger all need to be callable from the instrumentation code. The user code (instrumentation code) calls the GetLoggerProvider and GetLogger directly, so they need to be in the API package, so that we can provide a default noop implementation.

@open-telemetry/specs-logs-approvers is this your understanding as well?

@jkwatson
Copy link
Contributor

jkwatson commented Dec 15, 2022

I think what @trask was thinking (and what I also have been thinking) is that the Event API doesn't need to be so tightly bound to the logging API (which isn't intended for end-users!). I was thinking that we would be able to do something like:

  EventEmitter emitter = openTelemetry.getEventEmitter("myLibrary");
  emitter.emit("myevent", Attributes.of("foo", "bar"));

And that could be implemented in the SDK directly without having to have any (non-user-facing!) logging API interaction in order to emit events.

@tigrannajaryan
Copy link
Member Author

I think what @trask was thinking (and what I also have been thinking) is that the Event API doesn't need to be so tightly bound to the logging API (which isn't intended for end-users!). I was thinking that we would be able to do something like:

  EventEmitter emitter = openTelemetry.getEventEmitter("myLibrary");
  emitter.emit("myevent", Attributes.of("foo", "bar"));

And that could be implemented in the SDK directly without having to have any (non-user-facing!) logging API interaction in order to emit events.

This is of course possible, but it is a different Events API, not the one we have in the spec currently. IIRC we discussed an Events API does not rely on Logging API and it was rejected in the past. One of the reasons I remember was that it requires duplicating lots of the machinery (the Provider/EventEmitter/etc) to essentially produce the same data (a LogRecord) using a different looking API and we decided against it.

I am still totally fine with re-thinking the Events API, I am not particularly attached to the current one, so feel free to make complete proposals in a separate issue/PR.

@jkwatson
Copy link
Contributor

So, if we're "not building a Logging API" and the event API requires using the logging API (which we're not building), then it seems like we have to build a end-developer-facing event API. :)

Unfortunately, I am not being paid to be a maintainer any more, and don't have time to shepherd a whole new API through the specification process, but I sincerely hope that what will be built for events will be an end-user-facing API, and hence will not be associated, or require, usages of the non-end-user-facing logging API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue editorial Editorial changes only (typos, changelog, ...). No content-related changes of any kind. spec:logs Related to the specification/logs directory Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make sure it is very clear we are not building a Logging API
8 participants