trino icon indicating copy to clipboard operation
trino copied to clipboard

Implement MySQL based event listener

Open arhimondr opened this issue 2 years ago • 4 comments

Description

MySQL based event listener allows to log detailed information about queries running on a Trino cluster directly into a MySQL table

Is this change a fix, improvement, new feature, refactoring, or other?

New feature

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

New plugin

How would you describe this change to a non-technical end user or system administrator?

A plugin that allows to log information about queries running on a Trino cluster in MySQL table. The information include query text, query plan, detailed statistics and more. This information is useful for workload analysis and optimization.

Related issues, pull requests, and links

-

Documentation

( ) No documentation is needed. ( ) Sufficient documentation is included in this PR. ( ) Documentation PR is available with #prnumber. (x) Documentation issue #11807 is filed, and can be handled later.

Release notes

(x) No release notes entries required. ( ) Release notes entries required with the following suggested text:

# Plugins
* Add MySQL based event listener implementation

arhimondr avatar Mar 24 '22 04:03 arhimondr

Only the last commit. Still working on documentation.

arhimondr avatar Mar 24 '22 04:03 arhimondr

This implementation doesn't support automatic schema evolution. I may not be suitable for production use. However it provides an easy-to-setup way of collecting query runtime information that could be useful for testing.

@mosabua I created an issue for documentation (https://github.com/trinodb/trino/issues/11807). However I'm not sure whether it makes sense to advertise this implementation as it may not be suitable for production deployments. What do you think?

arhimondr avatar Apr 06 '22 02:04 arhimondr

@sopel39 YOu had some concerns regarding this one. Can you comment and/or approve?

losipiuk avatar Apr 11 '22 18:04 losipiuk

@sopel39 YOu had some concerns regarding this one. Can you comment and/or approve?

I don't think even listener is bad per se. I just wondered if verifier shouldn't obtain query JSON directly. Feel free to merge

sopel39 avatar Apr 11 '22 20:04 sopel39

@arhimondr do we still need it?

losipiuk avatar Nov 15 '22 11:11 losipiuk

I do. It's still very convenient when testing.

On Tue, Nov 15, 2022, 6:59 AM Łukasz Osipiuk @.***> wrote:

@arhimondr https://github.com/arhimondr do we still need it?

— Reply to this email directly, view it on GitHub https://github.com/trinodb/trino/pull/11645#issuecomment-1315208640, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABKQDLE5TBH3KETXLEP42ZDWIN3IVANCNFSM5RP6LP5A . You are receiving this because you were mentioned.Message ID: @.***>

arhimondr avatar Nov 15 '22 13:11 arhimondr

:wave: @arhimondr - Looks like there's activity here so we won't mess with this for now. Just wanted to show that we looked at this during the backlog sweep already.

bitsondatadev avatar Nov 19 '22 07:11 bitsondatadev

I've been using it for quite some time for testing. This plugin provides an easy way to collect QueryCompletedEvent's. I would still argue that we should consider having this merged.

arhimondr avatar Nov 21 '22 19:11 arhimondr

I agree with @arhimondr .. if it is useful for testing and development alone it is already worth merging. And it seems it can even be used beyond that. I support a merge. We can tackle documentation separately.

mosabua avatar Nov 22 '22 03:11 mosabua

The problem is once you add this people start using it for non-testing purposes and you end up with feature creep and maintenance. And once people start using it they'll update from one Trino version to another and since we don't provide migrations they'll complain that it's broken.

See FileHiveMetastore evolution for example on how it tackled that problem (explicitly disallowing a metastore created on version X to be used with any Trino version other than X).

hashhar avatar Nov 23 '22 07:11 hashhar

If there's a concern that this isn't suitable for production use and that feature creep maintenance is going to be a burden, I think it makes sense to skip publicizing/mentioning it for now. If we want developers to use it, we should reference it in development.md or a readme somewhere. If we decide it's suitable for general Trino users, we can add a release note once we're both confident in that and have documentation to guide their usage.

colebow avatar Nov 23 '22 10:11 colebow

Yeah, we don't have to document it for now. Yet it's very convenient for testing purposes. Also in a hypothetical situation (when it breaks) only the added functionality is lost (event loggin) and Trino itself is not impacted, while breaking functionality of the FileMetastore breaks basic functionality.

arhimondr avatar Nov 23 '22 17:11 arhimondr