Add a data source for Trino/Presto #1201

maxstreese · 2023-09-21T09:10:07Z

Description

This PR adds a data source for Trino/Presto.

Resolves #1095.

Adding Trino/Presto significantly increases the number of data sources supported by Evidence, as they function as data source aggregators.

Checklist

For UI or styling changes, I have added a screenshot or gif showing before & after
I have added a changeset

Screenshots

The new connection form with a valid connection

The new connection form with an invalid connection

A query running against the new data source

changeset-bot · 2023-09-21T09:10:11Z

🦋 Changeset detected

Latest commit: c931cdc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages

Name	Type
@evidence-dev/core-components	Patch
@evidence-dev/db-orchestrator	Patch
@evidence-dev/evidence	Patch
@evidence-dev/postgres	Patch
@evidence-dev/trino	Patch
evidence-docs	Patch
@evidence-dev/components	Patch
evidence-test-environment	Patch
@evidence-dev/redshift	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel · 2023-09-21T09:10:11Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
evidence-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 25, 2023 0:25am

netlify · 2023-09-21T09:10:12Z

✅ Deploy Preview for evidence-development-workspace ready!

Name	Link
🔨 Latest commit	`c931cdc`
🔍 Latest deploy log	https://app.netlify.com/sites/evidence-development-workspace/deploys/65117bdfc4029400080fb860
😎 Deploy Preview	https://deploy-preview-1201--evidence-development-workspace.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

maxstreese · 2023-09-21T14:02:20Z

Hi all!

Let me know if there is anything else I can do here at this point. For now I would consider this as final from my side

Best,
Max

maxstreese · 2023-09-21T20:02:56Z

Just thinking out loud, it may also be and idea to ask the Trino folks to be included in their list of supported clients once the datasource is supported by Evidence. Might attract some attention.

archiewood · 2023-09-22T03:01:50Z

Thanks for the contribution! 🙏🏼

I don't have any experience with ~~presto~~ trino, so I set up a free account on Starburst to give this a test with.

I ran into issues connecting:

The error message when connecting does not seem to get unpacked. Do we need to unpack this somehow?
Checking in the console, the error I'm specifically getting is around sending an HTTP request to an HTTPS port:

Object { message: "execution error:<html>\r\n<head><title>400 The plain HTTP request was sent to HTTPS port</title></head>\r\n<body>\r\n<center><h1>400 Bad Request</h1></center>\r\n<center>The plain HTTP request was sent to HTTPS port</center>\r\n<hr><center>cloudflare</center>\r\n</body>\r\n</html>\r\n", code: 400 }

Are there other config options I should be passing in here? I feel like starburst should be requiring some kind of password, but adding my starburst login pw didn't change the above behaviour.

Apologies if I'm being slow here!

maxstreese · 2023-09-22T06:26:37Z

Hey, awesome! You need to set SSL to "true" in this case. Then things should work (you also need to provide user and password). I tried this against my company's Trino installation and that worked.

My understanding is that HTTPS uses SSL/TLS so therefore requiring the user to set the SSL option for an HTTPS connection seems reasonable. It's also in line with how the client library underneath works. Additionally this allows for the freedom to run Trino in HTTPS mode but on a different port than 443. That being said, if you'd like me to silently set the SSL option for the user in case the port is 443, I could also do that.

Let me know 😀

maxstreese · 2023-09-22T10:34:51Z

Hi @archiewood,

I have made three changes now:

Unpack the error message as you suggested
Provide user instructions in the form for the SSL parameter (stating that it must be set for HTTPS connections)
Include an engine option which should allow this data source to be used for both Trino and Presto. The underlying client library allows for this. I have not tested against a Presto installation, but only against both a local (HTTP) as well as a remote (HTTPS) Trino installation

I have updated the PR title and description accordingly.

archiewood · 2023-09-22T14:20:26Z

Include an engine option which should allow this data source to be used for both Trino and Presto.

Sorry I misspoke in my above comment. Starburst is cloud hosted trino. I wasn't aware they were different aside from the naming tbh

So I've made progress, I now have a new error message after setting ssl=true

Again, this feels like a fair point, there seems to be no password or auth. So I threw in my starburst password. Still no dice:

I'm not really sure. Maybe I should be using a service account for this?
EDIT: No luck with a service account either

maxstreese · 2023-09-22T15:14:49Z

Sorry I misspoke in my above comment. Starburst is cloud hosted trino. I wasn't aware they were different aside from the naming tbh

No problem and you didn't. I was aware that Starburst is Trino. But I realized that it was silly to exclude Presto. So I added the engine option. This was not related to your comment at all.

So I threw in my starburst password. Still no dice

Ah my bad, that's embarrassing. I had a bug in the auth function. Unfortunately I do not have a local test setup for this but I hope with the changes I just pushed things should work. Please let me know!

archiewood · 2023-09-22T16:05:11Z

Hmm, still not getting there my side.
Now I'm getting a rather unhelpful, unspecified "execution error"

Unfortunately I do not have a local test setup for this but I hope with the changes I just pushed things should work. Please let me know!

FWIW you can set up a free starburst account in about 10 mins, which might help with testing

archiewood · 2023-09-22T16:09:29Z

my main sticking point is that I feel like trino should be authing me somehow. I can see if you have a local setup then maybe that's not an issue, but presumably any prod connection will need to be authed?

So why does starburst not give me any auth params - all these things are very guessable:

I'm guessing something to do with HTTPS/SSL/TLS solves this?

I feel like I'm missing something?

Maybe some clues

on the dbt trino page: https://docs.getdbt.com/docs/core/connect-data-platform/trino-setup,
or the trino page itself: https://trino.io/docs/current/security/authentication-types.html

maxstreese · 2023-09-22T17:08:08Z

Let me check later this weekend. Right now when user and password are both provided, I set the basic_auth property of the client library I use (presto-client-node). I believe this should be supported by Starburst somehow. I'll sign myself up for a free account as you suggested.

maxstreese · 2023-09-22T18:41:38Z

Hi @archiewood !

I actually think the implementation is working as is right now. I signed up with Starburst and this is what I got:

The password I provided is the regular password of my account.

If I then go ahead and create a sample site in the example project I get:

Finally, if I check the query history on Starburst I can see both, the query that Evidence uses to test the connection initially, as well as my sample query:

archiewood · 2023-09-22T19:46:34Z

Nice! It's also working for me!

I hadn't realised but the free trino clusters auto suspend after 15 mins. Weird that the error didn't tell me this. Feels like it should.

archiewood · 2023-09-22T20:03:02Z

Okay so having been through this process, I think it would be great to add a bit of docs help / explanation to go alongside this.

IIUC your connector only supports password authentication? (which is fine if so) - we should say this in the docs section for trino, as Trino has many auth methods: https://trino.io/docs/current/security/authentication-types.html
The other gotchas I had were using starburst specifically. I have no idea how popular starburst is vs other ways of running trino, but if its a meaningful percentage, we should maybe include a note on this too:

knowing that the password was required AND that the password was the one i use to login to the UI client, rather than some other key
knowing that I should set SSL=true

more generally - should the password be required? Or are there circumstances where it is not (eg local dev)

maxstreese · 2023-09-25T09:13:42Z

1. RE: Authentication Methods

Correct, my implementation only supports password (i.e. basic) authentication. Two comments about this below.

The limitation here is in the available Trino client library for Node/JS, namely presto-client-node. As this does currently only support password authentication, so does my connector. One could theoretically either implement a fully custom Trino client for Evidence, or extend presto-client-node to support more authentication methods. I am currently not willing/able to do this.

Looking at the data consumer list provided by Starburst, you will find that quite a few of the available Trino clients actually only support the password based authentication types. So I think that Evidence would be in good company here.

2. RE: Should The Password Be Required?

No. There are setups besides local development where you do not need a password. You can for instance self host Trino and give access via VPN.

3. RE: Starburst Specific Documentation

I have no clue how many companies use Starburst vs how many self host but it certainly can't hurt to include information about Starburst in the docs.

The password is not necessarily the one you use to log in to the Starburst UI by the way. You could also create a dedicated service account and use that one for connecting:

4. Providing Documentation

Let me try to come up with something

maxstreese · 2023-09-25T13:00:50Z

Done. Let me know if I should add/change something.

archiewood · 2023-09-25T21:40:05Z

Looks great - thanks for the contribution!

mcrascal · 2023-09-25T21:46:11Z

Amazing! Thanks @maxstreese @archiewood 🎉

maxstreese · 2023-09-26T08:30:52Z

Awesome, thanks! In case anything turns out to be amiss just let me know.

Add a datasource for Trino (formerly PrestoDB)

86e7dda

vercel bot temporarily deployed to Preview September 21, 2023 09:11 Inactive

Run 'pnpm install' and 'pnpm format'

18e3c46

vercel bot temporarily deployed to Preview September 21, 2023 10:01 Inactive

Make Trino password optional

2202a17

vercel bot temporarily deployed to Preview September 21, 2023 12:59 Inactive

Merge branch 'main' into trino-datasource

db5ba95

vercel bot temporarily deployed to Preview September 21, 2023 13:35 Inactive

Add a changeset

c2540d2

maxstreese changed the title ~~WIP: Add a datasource for Trino (formerly PrestoDB)~~ Add a datasource for Trino (formerly PrestoDB) Sep 21, 2023

vercel bot temporarily deployed to Preview September 21, 2023 13:57 Inactive

Improve error reporting; Add 'engine' option; Add more instructions

a47ca01

maxstreese changed the title ~~Add a datasource for Trino (formerly PrestoDB)~~ Add a datasource for Trino/PrestoDB Sep 22, 2023

vercel bot temporarily deployed to Preview September 22, 2023 10:26 Inactive

maxstreese changed the title ~~Add a datasource for Trino/PrestoDB~~ Add a datasource for Trino/Presto Sep 22, 2023

maxstreese changed the title ~~Add a datasource for Trino/Presto~~ Add a data source for Trino/Presto Sep 22, 2023

Fix a bug with setting the basic_auth

441f7ac

vercel bot temporarily deployed to Preview September 22, 2023 15:13 Inactive

Provide more documentation

7899593

vercel bot temporarily deployed to Preview September 25, 2023 12:06 Inactive

Fix a typo

c931cdc

vercel bot temporarily deployed to Preview September 25, 2023 12:25 Inactive

archiewood merged commit 84cf521 into evidence-dev:main Sep 25, 2023
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a data source for Trino/Presto #1201

Add a data source for Trino/Presto #1201

maxstreese commented Sep 21, 2023 •

edited

changeset-bot bot commented Sep 21, 2023 •

edited

vercel bot commented Sep 21, 2023 •

edited

netlify bot commented Sep 21, 2023 •

edited

maxstreese commented Sep 21, 2023

maxstreese commented Sep 21, 2023

archiewood commented Sep 22, 2023 •

edited

maxstreese commented Sep 22, 2023 •

edited

maxstreese commented Sep 22, 2023 •

edited

archiewood commented Sep 22, 2023 •

edited

maxstreese commented Sep 22, 2023

archiewood commented Sep 22, 2023

archiewood commented Sep 22, 2023 •

edited

maxstreese commented Sep 22, 2023

maxstreese commented Sep 22, 2023

archiewood commented Sep 22, 2023 •

edited

archiewood commented Sep 22, 2023

maxstreese commented Sep 25, 2023 •

edited

maxstreese commented Sep 25, 2023

archiewood commented Sep 25, 2023

mcrascal commented Sep 25, 2023

maxstreese commented Sep 26, 2023

Add a data source for Trino/Presto #1201

Add a data source for Trino/Presto #1201

Conversation

maxstreese commented Sep 21, 2023 • edited

Description

Checklist

Screenshots

The new connection form with a valid connection

The new connection form with an invalid connection

A query running against the new data source

changeset-bot bot commented Sep 21, 2023 • edited

🦋 Changeset detected

vercel bot commented Sep 21, 2023 • edited

netlify bot commented Sep 21, 2023 • edited

✅ Deploy Preview for evidence-development-workspace ready!

maxstreese commented Sep 21, 2023

maxstreese commented Sep 21, 2023

archiewood commented Sep 22, 2023 • edited

maxstreese commented Sep 22, 2023 • edited

maxstreese commented Sep 22, 2023 • edited

archiewood commented Sep 22, 2023 • edited

maxstreese commented Sep 22, 2023

archiewood commented Sep 22, 2023

archiewood commented Sep 22, 2023 • edited

maxstreese commented Sep 22, 2023

maxstreese commented Sep 22, 2023

archiewood commented Sep 22, 2023 • edited

archiewood commented Sep 22, 2023

maxstreese commented Sep 25, 2023 • edited

1. RE: Authentication Methods

2. RE: Should The Password Be Required?

3. RE: Starburst Specific Documentation

4. Providing Documentation

maxstreese commented Sep 25, 2023

archiewood commented Sep 25, 2023

mcrascal commented Sep 25, 2023

maxstreese commented Sep 26, 2023

maxstreese commented Sep 21, 2023 •

edited

changeset-bot bot commented Sep 21, 2023 •

edited

vercel bot commented Sep 21, 2023 •

edited

netlify bot commented Sep 21, 2023 •

edited

archiewood commented Sep 22, 2023 •

edited

maxstreese commented Sep 22, 2023 •

edited

maxstreese commented Sep 22, 2023 •

edited

archiewood commented Sep 22, 2023 •

edited

archiewood commented Sep 22, 2023 •

edited

archiewood commented Sep 22, 2023 •

edited

maxstreese commented Sep 25, 2023 •

edited