Skip to content

Conversation

@losipiuk
Copy link
Member

@losipiuk losipiuk commented May 27, 2024

Description

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# Section
* Add event listener which exposes collected events on HTTP endpoint. ({issue}`22158`)

@cla-bot cla-bot bot added the cla-signed label May 27, 2024
@losipiuk losipiuk requested a review from findepi May 27, 2024 10:54
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

this is not a listener. How does one get the query id in the first place to query the listener?

@findepi
Copy link
Member

findepi commented May 27, 2024

query id is not a provide thing. eg from io.trino.jdbc.TrinoResultSet#getStats

@losipiuk
Copy link
Member Author

this is not a listener. How does one get the query id in the first place to query the listener?

You can list ids - for one (there is a listing endpoint) . But you may also just know it - get it from on of the clients which sent queries to the server.

@losipiuk losipiuk force-pushed the http-resource-event-listener branch from 06569b0 to 252ad90 Compare May 27, 2024 15:28
@losipiuk
Copy link
Member Author

@hashhar are we good with this one?

@hashhar hashhar dismissed their stale review May 27, 2024 17:40

I can see how this can work but don't see much utility in this over the other mechanisms. Specially since this adds a non-trivial amount of work on the coordinator (query events are large and serialization is costly).

@losipiuk
Copy link
Member Author

I can see how this can work but don't see much utility in this over the other mechanisms. Specially since this adds a non-trivial amount of work on the coordinator (query events are large and serialization is costly).

Direct usecase here is to integrate query-completion-events in benchto which we use for performance testing of Trino.

@losipiuk losipiuk merged commit f6662c1 into trinodb:master May 27, 2024
@github-actions github-actions bot added this to the 449 milestone May 27, 2024
@hashhar
Copy link
Member

hashhar commented May 28, 2024

Thanks for the context, that's helpful and makes sense.

@mosabua
Copy link
Member

mosabua commented Jun 1, 2024

Where are the docs for this and how does it compare to

https://trino.io/docs/current/admin/event-listeners-http.html

Something is messed up here ..

Seems like its not really a event listener that it sends the events somewhere .. more like it just makes them available .. we need to document that somehow

@losipiuk
Copy link
Member Author

losipiuk commented Jun 3, 2024

Seems like its not really a event listener that it sends the events somewhere .. more like it just makes them available .. we need to document that somehow

Yeah - it listens internally on event and exposes data over http. We may document it if you think it is useful for end-users. So far intention was mostly to be able to expose query completion events to benchto performance testing framework.

@mosabua
Copy link
Member

mosabua commented Jun 3, 2024

hm .. we added it in the release notes @martint and @colebow but I am not sure this really is an event listener in our sense in the docs .. I am also not sure if there is a user facing scenario that warrants documenting this feature, but there might well be. I am inclined to add some minimal docs and leave the release notes entry as is.

wdyt @losipiuk @martint @colebow @hashhar

@oneonestar
Copy link
Member

Haven't tried this yet, but I saw no authentication for these endpoints.
I don't think this is user facing without auth.

@hashhar
Copy link
Member

hashhar commented Jun 4, 2024

This is meant to be used with benchto only - not user facing. See trinodb/benchto#73

@mosabua
Copy link
Member

mosabua commented Jun 4, 2024

This is meant to be used with benchto only - not user facing. See trinodb/benchto#73

So it should be removed from the release notes then.. since its an internal implementation detail. Right?

@hashhar
Copy link
Member

hashhar commented Jun 4, 2024

IMO yes.

@losipiuk
Copy link
Member Author

losipiuk commented Jun 4, 2024

IMO yes.

fine by me too

@mosabua
Copy link
Member

mosabua commented Jun 4, 2024

#22262

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

5 participants