[o11y][nfc] Require getEventInfo() to be defined to ensure EventInfo is present

[fhanau]

Downstream PR to follow.

[mar-cf]

getEventInfo calls were never added for non-custom Events

This concern isn't applicable here. They don't use the interface. If we wanted to unify for all event types, we could either consolidate them into a common interface or add a similar pattern to whatever abstraction they share, if any.

For consistency, invoke setEventInfo() directly. It is sufficient to do so before delivered() is called.

I might have found a few places where this wasn't the case. But this change feels like a step in the wrong direction. The getEventInfo() pattern provided a natural extension point that prompted implementers to make an explicit decision about tracing. I'd prefer we make getEventInfo() pure virtual instead of removing it - this would force every new CustomEvent to explicitly decide what trace info to provide (even if that decision is kj::none). With manual setEventInfo() calls scattered, it's easy to forget entirely. Implementers have to know when to call it (before delivered()) rather than having that handled automatically. There's no compile-time enforcement.

[fhanau]

getEventInfo calls were never added for non-custom Events

This concern isn't applicable here. They don't use the interface. If we wanted to unify for all event types, we could either consolidate them into a common interface or add a similar pattern to whatever abstraction they share, if any.

For consistency, invoke setEventInfo() directly. It is sufficient to do so before delivered() is called.

I might have found a few places where this wasn't the case. But this change feels like a step in the wrong direction. The getEventInfo() pattern provided a natural extension point that prompted implementers to make an explicit decision about tracing. I'd prefer we make getEventInfo() pure virtual instead of removing it - this would force every new CustomEvent to explicitly decide what trace info to provide (even if that decision is kj::none). With manual setEventInfo() calls scattered, it's easy to forget entirely. Implementers have to know when to call it (before delivered()) rather than having that handled automatically. There's no compile-time enforcement.

I still think this is unnecessarily bulky, but making getEventInfo() pure virtual is some progress at least. Replaced this change to do just that and also make it return tracing::EventInfo directly, since we always need the Onset event to be present.
I think a cleaner way to enforce having the event would be to add it as a required parameter in delivered() itself, but to avoid superfluous memory allocations that would have to provided through a lambda which is more of a hassle than I'm willing to deal with right now.

[codspeed-hq]

CodSpeed Performance Report

Merging #5798 will degrade performance by 11.07%

_{Comparing felix/100925-fix-event-reporting (67545b5) with main (49feab0)}

Summary

❌ 1 regression
✅ 139 untouched
⏩ 38 skipped¹

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	BASE	HEAD	Efficiency
❌	Encode_ASCII_256[TextEncoder][0/0/256]	3.1 ms	3.5 ms	-11.07%

Footnotes

1. 38 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩