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

Rename EventLogger to EventEmitter #3869

Closed

Conversation

martinkuba
Copy link
Contributor

Changes

This updates the Events API spec to use EventEmitter instead of EventLogger. Using emitter seems to be more fitting in the context of emitting events. Both the Java and JavaScript prototypes have used EventEmitter.

@martinkuba martinkuba requested review from a team as code owners February 7, 2024 22:29
@martinkuba martinkuba changed the title Eename EventLogger to EventEmitter Rename EventLogger to EventEmitter Feb 7, 2024
@carlosalberto
Copy link
Contributor

(Btw I realized we don't seem to have an actual event WG to ping for such PRs)

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.

I like it. 🙏🏻

@martinkuba
Copy link
Contributor Author

@MrAlias brought up a suggestion to use a single-term name to be consistent with other APIs (Tracer, Logger, Meter).
#3878 (comment)

Options currently considered:

  • EventLogger (keep as is)
  • EventEmitter (proposed here)
  • Eventer (proposed by @MrAlias )

@tedsuo
Copy link
Contributor

tedsuo commented Feb 23, 2024

I hate typing and I would love a shorter name but I'd prefer it be one that is standard and that the community does not find ridiculous. Tracer and Logger are standard terms. In metrics we went with Meter and it confuses everyone. There is no way in hell we are going with Eventer (sorry Tyler 🙂).

I would prefer longer and standard over shorter and weird. I disagree that there is a requirement that the name be a single word. If someone can come up with a good one, great! Otherwise we should go with the standard industry term, which in this case is EventEmitter.

@tedsuo
Copy link
Contributor

tedsuo commented Feb 23, 2024

And to put my money where my mouth is, I think I have a solution!

I suggest we just go with Event.

  • EventProvider.Get()
  • Event.Emit()

That API feels good to me. And if we ever need an object to describe an individual event, we could go with EventRecord which conforms with LogRecord.

@MrAlias
Copy link
Contributor

MrAlias commented Feb 23, 2024

My concern with EventEmitter and EventLogger is more about them containing an operation in their name than a single word. Extending them with other operations is going to be problematic given they already self-describe their only operation.

As for Eventer not being "standard". Just remember, at one point "Logger" and "Tracer" were non-standard terms. It's only from the vantage point of their full adoption that you now see them as "standard".

@MrAlias
Copy link
Contributor

MrAlias commented Feb 23, 2024

@tedsuo also consider this. The word Event already has a definition. Using that as a term here will bring with it that definition. For the same reason people may question why we used Meter, they will question why we used a noun that refers to a thing that happens instead of something that describes a tool to record events.

When we invent a word like Eventer we can assign what it means. The user will have the context that it relates to events but they will not be thrown off with any preconceived definitions of the word.

@tedsuo
Copy link
Contributor

tedsuo commented Feb 23, 2024

I think people simply question why we didn't use the word Metric. 🙂 (I understand the reasons why we didn't.) but even there, we didn't use a made up word. Meter is defined in the dictionary as "a device that measures and records the quantity, degree, or rate of something."

It's true that the industry made up the word "logger" at some point. But in this case, the industry made up the word "EventEmitter."

@MrAlias
Copy link
Contributor

MrAlias commented Feb 23, 2024

It's true that the industry made up the word "logger" at some point. But in this case, the industry made up the word "EventEmitter."

Agreed. So if we are going to make up a word to capture a new concept let's make sure that word is not a compound that only defines one operation. Similar to Tracer.

@breedx-splk
Copy link
Contributor

I'm fine with both EventEmitter and Eventer, tho I respect the silly confusion related Meter thing raised by @tedsuo . They're both less leaky abstractions than EventLogger.

My concern with EventEmitter and EventLogger is more about them containing an operation in their name than a single word.

I don't think EventEmitter is confusing in the slightest, in fact, I think it's very clear. If you have an EventEmitter it is very clear what its purpose is and what it does -- it emits events. It's right there in the name.

Extending them with other operations is going to be problematic given they already self-describe their only operation.

I'm not at all concerned with extending EventEmitter with other operations, because I'd rather promote the single responsibility principle (SRP), so that other future operations exist on separate interfaces. Like, you can imagine what an EventAggregator does, and you can imagine what an EventFilter does, just like you might also guess what an EventSchemaValidator does. None of those things exist yet, and may never, but they're similarly obvious in purpose.

If we decide in the future (for ergonomic or aesthetic reasons) that we need to combine these separate responsibilities, we should fight about the name then (at that time) and use the composite pattern. I speculate that at that future time the name will be more clear -- because it would be a more top-level grouping responsibility.

@MrAlias
Copy link
Contributor

MrAlias commented Feb 26, 2024

My concern with EventEmitter and EventLogger is more about them containing an operation in their name than a single word.

I don't think EventEmitter is confusing in the slightest, in fact, I think it's very clear. If you have an EventEmitter it is very clear what its purpose is and what it does -- it emits events. It's right there in the name.

Extending them with other operations is going to be problematic given they already self-describe their only operation.

I'm not at all concerned with extending EventEmitter with other operations, because I'd rather promote the single responsibility principle (SRP), so that other future operations exist on separate interfaces. Like, you can imagine what an EventAggregator does, and you can imagine what an EventFilter does, just like you might also guess what an EventSchemaValidator does. None of those things exist yet, and may never, but they're similarly obvious in purpose.

If we decide in the future (for ergonomic or aesthetic reasons) that we need to combine these separate responsibilities, we should fight about the name then (at that time) and use the composite pattern. I speculate that at that future time the name will be more clear -- because it would be a more top-level grouping responsibility.

@breedx-splk this all sounds fine, but the EventEmitter approach does not take into account OTel's existing signals, policies, and practices.

  • We already name our types unified composites: Tracer, Logger, and Meter. Adopting a different naming practice on our forth signal will be inconsistent.
    • I'm not sure how much clearer it could be that we have already chosen to name something Logger and not LogEmitter, but we would choose to name something EventEmitter now?
  • We already have a policy to evolve our APIs by extending existing APIs12, not by adding adding interfaces and adding more combination interfaces. Designing our forth signal to evolve in a different way will split how OTel evolves. We should not do this, and we should not start building the signal with the understanding that we will do this in the future when the names we choose now don't fit the existing evolution strategy.

Additionally, the evolution strategy you mention would not just result in EventEmitter, EventAggregator, EventFilter, and EventSchemaValidator, but it would also require EventEmitterProvider, EventAggregatorProvider, EventFilterProvider, and EventSchemaValidatorProvider. On top of that, any "combination" type would also require a provider. This really highlights the difference between with existing signals. When we added methods to existing interfaces in existing signals we are not required to add additional providers.

Footnotes

  1. https://github.com/open-telemetry/opentelemetry-specification/pull/3678

  2. https://github.com/open-telemetry/opentelemetry-specification/pull/3540

@tedsuo
Copy link
Contributor

tedsuo commented Feb 26, 2024

We'll try to present a conclusion at tomorrow's spec meeting. But to clarify some things:

We don't have a consistent practice of unified composites. We have a consistent practice of using industry standard terminology. The short names are just a coincidence. We wouldn't use LogEmitter because that is not standard practice in the industry.

In the case of Meter, we used that name it because we wanted to avoid using the term Metric to describe two different objects, not because we wanted to conform to a convention. My Event proposal above is rejected for the same reason. However, in the case of events we do not actually have this double naming problem, as the term EventEmitter is standard practice.

Emitting events will always be the primary purpose of the EventEmitter API. We have never created an instrumentation API that did anything other than record a specific type of data. All of the suggested possibilities like EventFilter are exactly the type of thing we would never put in an instrumentation API. Besides an emitter, the only other public API in an event system is an EventListener, which we are intentionally not adding as our system it is only for recording telemetry, not a full event system to be leveraged by an application for other purposes. The need for additional APIs for recording events is an unreasonable assumption to make.

However, if we must leave the door open for additional APIs unrelated to event recording – a requirement not made on any other instrumentation type – I suggest we solve this by having an EventProvider instead of an EventEmitterProvider. The EventProvider then has a GetEventEmitter method. This allows the EventEmitter to remain a single purpose API even if we want additional APIs later.

@MrAlias
Copy link
Contributor

MrAlias commented Feb 27, 2024

However, in the case of events we do not actually have this double naming problem, as the term EventEmitter is standard practice.

Can you please provide references? What telemetry platform uses the term EventEmitter?

@MrAlias
Copy link
Contributor

MrAlias commented Feb 27, 2024

Emitting events will always be the primary purpose of the EventEmitter API.

How are you able to say this with confidence? This is unsubstantiated.

@MrAlias
Copy link
Contributor

MrAlias commented Feb 27, 2024

I suggest we solve this by having an EventProvider instead of an EventEmitterProvider. The EventProvider then has a GetEventEmitter method. This allows the EventEmitter to remain a single purpose API even if we want additional APIs later.

This goes even further away from our standards. Your proposal now includes a provider named in a way that does not include in it what it provides. How do we justify EventProvider when we already have a TracerProvider that provides a Tracer, a MeterProvider that provides a Meter, and LoggerProvider that provides a Logger?

@MrAlias
Copy link
Contributor

MrAlias commented Feb 27, 2024

We don't have a consistent practice of unified composites. We have a consistent practice of using industry standard terminology. The short names are just a coincidence. We wouldn't use LogEmitter because that is not standard practice in the industry.

Your view here is only one way to look at this. We don't use LogEmitter, nor does the rest of the industry, because it our Logger may perform more operations than just emitting in the future.

Furthermore, it is not a valid argument to say that there are no industry standard terms for an what we want to call an Eventer and then use industry standard terms as an argument for not going in that direction.

@MrAlias
Copy link
Contributor

MrAlias commented Feb 27, 2024

We'll try to present a conclusion at tomorrow's spec meeting.

I'm not sure a conclusion is the stage of this conversation we are at. There are many points that I have made within this thread that have not be directly responded to. These points have instead been ignored and unjustifiably the term EventEmitter continues to be used.

@MrAlias
Copy link
Contributor

MrAlias commented Feb 27, 2024

The EventProvider then has a GetEventEmitter method. This allows the EventEmitter to remain a single purpose API even if we want additional APIs later.

Again, like mentioned above, this is not how we have historically evolved our APIs. Why, on our forth signal, should we do something different here?

@tedsuo
Copy link
Contributor

tedsuo commented Feb 27, 2024

Following up from the spec meeting, we decided to collect up a list of name proposals, and hand them to the TC next week to make a final decision. Anyone who would like to proposal a name along with a justification, please do so here before March 5th.

@trask
Copy link
Member

trask commented Feb 27, 2024

So far, these names have been proposed (in alphabetical order):

  • EventEmitter
  • Eventer
  • EventLogger
  • EventProducer
  • EventSink

@martinkuba
Copy link
Contributor Author

So far, these names have been proposed (in alphabetical order):

  • EventEmitter
  • Eventer
  • EventLogger
  • EventSink

EventProducer was also mentioned

@martinkuba
Copy link
Contributor Author

The recommendation from the Event WG was in order of preference:

  • EventLogger
  • EventEmitter

Decision was made on 3/5/24 in the Specification SIG to keep the name as EventLogger.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants