Skip to content

Comments

add convenience event emitting api to OpenTelemetryRum#892

Merged
breedx-splk merged 1 commit intoopen-telemetry:mainfrom
cleverchuk:cc/event-emit-api
Apr 18, 2025
Merged

add convenience event emitting api to OpenTelemetryRum#892
breedx-splk merged 1 commit intoopen-telemetry:mainfrom
cleverchuk:cc/event-emit-api

Conversation

@cleverchuk
Copy link
Contributor

Fixes #573

@cleverchuk cleverchuk requested a review from a team as a code owner March 17, 2025 13:01
*
* @param eventName The name of the event to emit.
*/
default void emitEvent(String eventName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about doing this in Kotlin and collapsing all these methods into one and use default arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it looks like that's very possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: it won't be very nice to Java users though.

Copy link
Member

Choose a reason for hiding this comment

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

you can add the annotation @JvmOverloads so kotlin generates overloads for the java callers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like that would require making the function a class method instead of being declare in an interface?

Copy link
Member

Choose a reason for hiding this comment

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

you can just do method overloads with kotlin anyway so its ok to convert it to kotlin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh true that, you can make this interface an abstract class but not sure if that's what you want

i was thinking the same. however, i don't know whether others would want it to changed to a class.

you can just do method overloads with kotlin anyway so its ok to convert it to kotlin

it looks like that defeats the purpose of converting to kotlin?

Copy link
Member

Choose a reason for hiding this comment

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

it looks like that defeats the purpose of converting to kotlin?

i think we all want at some point that the codebase is kotlin only when it makes sense.
all new code ideally is kotlin, if its not a big pain.

your call here since it works already

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe for now just keep this in Java and we can convert the whole class to Kotlin at a later point. There might be some binary compatibility issues in that case, but we can do it in a version bump where doing some trivial compile time breaks are ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good

@breedx-splk
Copy link
Contributor

I'm pretty certain this will need to change to align with sending data via attributes vs. body, but this has been baking out here long enough that I think we should merge it and improve/fix as needed.

@breedx-splk breedx-splk merged commit ec154db into open-telemetry:main Apr 18, 2025
6 checks passed
@cleverchuk cleverchuk deleted the cc/event-emit-api branch June 17, 2025 14:41
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.

Make emitting custom events simple

4 participants