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

Add Provider to the Event API #3878

Merged
merged 17 commits into from
Mar 12, 2024

Conversation

martinkuba
Copy link
Contributor

@martinkuba martinkuba commented Feb 15, 2024

Fixes #3086

Changes

The Event API will be used by instrumentations to generate events. Currently, the API takes an instance of a logger as a direct dependency. This means that instrumentations would need to be aware of the Log API, which is in conflict with the intent of using the Log API only to bridge logging frameworks. Furthermore, each instrumentation would need to construct an instance of an EventLogger, which is also inconsistent with the pattern of using a globally-registered provider.

This PR adds the EventLoggerProvider class to the API, making it consistent with other APIs.

@martinkuba martinkuba requested review from a team February 15, 2024 01:01
@tigrannajaryan
Copy link
Member

@martinkuba can you clarify what is the final intent? Do you plan to have an Events SDK that is configured separately from Logs SDK? Will Events SDK have its own pipeline of processors and exporters independently from Logs? Or is the plan to still rely on Logs SDK in some way?

@martinkuba
Copy link
Contributor Author

@martinkuba can you clarify what is the final intent? Do you plan to have an Events SDK that is configured separately from Logs SDK? Will Events SDK have its own pipeline of processors and exporters independently from Logs? Or is the plan to still rely on Logs SDK in some way?

The intent is to remove the direct dependency on the Log API from the Event API. Introducing the provider is a first step, and as I understood the outcome of #3086.

I agree that we need to specify how the API should be implemented as the next step. There were a few proposals in #3086. I think we do need to add a spec for Event SDK. My proposal would be to document that the default Event SDK should (or must) be implemented using the Log API. This would make it clear that, from OTel perspective, events are built on top of logs. And the Event SDK would not need to introduce any other concepts such as its own processors/exporters. I can add it to this PR, and I am also open to other suggestions.

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

There are structural specification issues with this PR that need to be addressed.

specification/logs/event-api.md Outdated Show resolved Hide resolved
specification/logs/event-api.md Outdated Show resolved Hide resolved
specification/logs/event-api.md Outdated Show resolved Hide resolved
specification/logs/event-api.md Show resolved Hide resolved
specification/logs/event-api.md Outdated Show resolved Hide resolved
Copy link
Contributor

@tedsuo tedsuo left a comment

Choose a reason for hiding this comment

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

Thank you for conforming to the spec and going with a Provider.

@martinkuba martinkuba changed the title Add EventEmitterProvider to the Event API Add Provider to the Event API Feb 27, 2024
Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

This seems like a good start to me.

Co-authored-by: jason plumb <75337021+breedx-splk@users.noreply.github.com>
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

🚀

@carlosalberto carlosalberto merged commit dd5a87a into open-telemetry:main Mar 12, 2024
7 checks passed
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
Fixes
open-telemetry#3086

## Changes

The Event API will be used by instrumentations to generate events.
Currently, the API takes an instance of a logger as a direct dependency.
This means that instrumentations would need to be aware of the Log API,
which is in conflict with the intent of using the Log API only to bridge
logging frameworks. Furthermore, each instrumentation would need to
construct an instance of an EventLogger, which is also inconsistent with
the pattern of using a globally-registered provider.

This PR adds the `EventLoggerProvider` class to the API, making it
consistent with other APIs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Further decouple Events API from Logs/Logger