Event emitter provider #5049
Event emitter provider #5049
Conversation
Codecov ReportBase: 91.00% // Head: 91.00% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #5049 +/- ##
=========================================
Coverage 91.00% 91.00%
+ Complexity 4899 4886 -13
=========================================
Files 549 550 +1
Lines 14538 14526 -12
Branches 1393 1382 -11
=========================================
- Hits 13230 13220 -10
- Misses 906 913 +7
+ Partials 402 393 -9
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
api/events/src/main/java/io/opentelemetry/api/events/EventEmitter.java
Outdated
Show resolved
Hide resolved
| * particular event domain, event name defines a particular class or type of event. | ||
| * @return a Logger instance. | ||
| */ | ||
| default EventEmitter get(String instrumentationScopeName, String eventDomain) { |
There was a problem hiding this comment.
Is eventDomain mandatory? I don't even know what sort of values I'd put in for this.
There was a problem hiding this comment.
As of now event.domain is required. The spec describes it as:
The event.domain attribute is used to logically separate events from different systems. For example, to record Events from browser apps, mobile apps and Kubernetes, we could use browser, device and k8s as the domain for their Events. This provides a clean separation of semantics for events in each of the domains. Within a particular domain, the event.name attribute identifies the event. Events with same domain and name are structurally similar to one another.
There's an open issue to discuss whether event.domain and event.name can be merged into a single field.
There was a problem hiding this comment.
This is the only sticking point I have with this API. I don't love having to explain to people what this is supposed to be used for. We have had enough trouble explaining what the "name" of a Tracer is...this adds an additional layer of having to explain this arcane idea that almost no one will care about. Can we make it optional? Or default to something like "none" or "unknown" or "who cares?"? :)
There was a problem hiding this comment.
I'm not opposed to making it optional with a default given the experimental state of the spec. The spec could reaffirm the importance and requirement of this field, in which case we'd have to come back and adjust our API, but that's fine and won't be the last breaking change to the event API. Will push a commit.
There was a problem hiding this comment.
Pushed a commit where event domain is now optional, set via eventEmitterProvider.eventEmitterBuilder("scope-name").setEventDomain("event-domain").build().
If unset, the event.domain attribute defaults to unknown.
…y-java into event-emitter-provider
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
…y-java into event-emitter-provider
|
@jack-berg do I understand correctly that with this PR we end up with roughly the following modules and data flows: Question: do we need the "Log API" in the API package or we can get rid of it and make Bridges/Appenders call directly into Log SDK package? |
This was actually our first approach (and the one I strongly favored). Unfortunately, getting this to work with the OTel javaagent ended up being quite problematic (@trask can probably explain the details, since it's pretty fuzzy in my head), leading us to create an API surface for the javaagent to interact with. |
Yes that's correct.
@tigrannajaryan I responded to the question of whether or not we need a log appender API here. In summary, yes, we do need a log appender API. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Split out the event API so that it is completely separate from the log API, rather than dependent on the log API as in #4961.
Summary of changes:
:api:eventsmodule, containing::api:logs:sdk:logsto implement both log API and event API. I.e.:SdkLoggerProviderimplements bothLoggerProviderandEventEmitterProviderSdkLoggerBuilderimplements bothLoggerBuilderandEventEmitterBuilderSdkLoggerimplements bothLoggerandEventEmitterGlobalEventEmitterProvider.setDraft for now as this is not what is currently described in the Event API spec. This approach is suggested by @trask and @jkwatson here, and by myself a while ago in the otep.