Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: display the socket path during migration #24222

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

brian-dlee
Copy link

@brian-dlee brian-dlee commented May 16, 2024

In this update, I've modified the migrate tool to display the socket path if it configured in the database connection. The problem that this solves is, for organizations like mine that use Google's Cloud SQL, our connection strings always include localhost:3306 and that actually does not indicate the true target database. Using Google cloud-sql-proxy, a unix socket is created to facilitate encrypted traffic to the database with Google IAM integration. This socket is named after the database you are connected to (e.g. orgname:us-west2:db-instance-name). In our adoption of Prisma, we've actually had accidents occur where someone runs a migration against a cloud environment thinking they are doing it locally since the socket name is not advertised.

An assumption I made in this PR is that any time a socket is supplied in the URL, it is the key indicator of the target database and completely takes over for host:port as the displayed identifier.

I would have liked to update the underlying prisma client connection messaging as well because database errors uninformatively indicate a connection failure to localhost:3306 and omit the socket name which is more important when debugging connectivity issues. I wasn't able to easily track down where this code lives and I was afraid of the widespread use of this. Truthfully, I'm way more worried about incorrectly applying migrations or performing database resets then debugging the Prisma client at runtime.

@CLAassistant
Copy link

CLAassistant commented May 16, 2024

CLA assistant check
All committers have signed the CLA.

@brian-dlee brian-dlee marked this pull request as ready for review May 16, 2024 18:44
@brian-dlee brian-dlee requested a review from a team as a code owner May 16, 2024 18:44
@brian-dlee brian-dlee requested review from SevInf and removed request for a team May 16, 2024 18:44
@janpio
Copy link
Member

janpio commented May 16, 2024

That sounds pretty useful. Can you share an (of course anonymized) example of what such a connection string effectively looks like?

(It seems that for PostgreSQL the "socket" is actually supplied via host parameter: https://www.prisma.io/docs/orm/overview/databases/postgresql#connecting-via-sockets)

What "underlying prisma client connection messaging" do you mean? Most probably it comes from the Prisma engines (query and schema engine) with their codebases at https://github.com/prisma/prisma-engines.

@brian-dlee
Copy link
Author

brian-dlee commented May 17, 2024

@janpio Here's an anonymized output from me running the CLI after my changes are applied.

node ./packages/cli/build/index.js migrate dev
Prisma schema loaded from prisma/schema.prisma
Datasource "db": MySQL database "my-db-name" at "unix:/Users/my-username/.cloudsql/my-project-name:us-west1:my-instance-name"

Error: P1017: Server has closed the connection.

The "underlying prisma client connection messaging" I meant is the error messaging that occurs if a database connection cannot be established. If we ever have problems with Prisma connecting to the database from Google Cloud Run we see error messages like "unable to connect to database at localhost:3306". If these error messages included the socket name (when appropriate) it could mean easier debugging for us. Maybe, for instance, we configured the incorrect cloud sql mount. It makes sense that I couldn't find it given that it's in the other repository 😅 .

@@ -210,7 +210,9 @@ export function getDbLocation(credentials: DatabaseCredentials): string | undefi
return credentials.uri!
}

if (credentials.host && credentials.port) {
if (credentials.socket) {
Copy link
Member

Choose a reason for hiding this comment

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

This should also include the host query parameter for PostgreSQL: https://www.prisma.io/docs/orm/overview/databases/postgresql#connecting-via-sockets Not sure how/if this is accessible here as credentials.host seems to be the parsed hostname though.

Copy link
Author

Choose a reason for hiding this comment

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

I'll handle the socket path supplied as host for PostgreSQL here. I think given that it's a unix socket I'll look for a host value that appears to be a unix-style path pattern.

Copy link
Author

Choose a reason for hiding this comment

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

All set

@janpio
Copy link
Member

janpio commented May 21, 2024

So localhost:3306 becomes unix:/Users/my-username/.cloudsql/my-project-name:us-west1:my-instance-name, where unix: is hardcoded and /Users/my-username/.cloudsql/my-project-name:us-west1:my-instance-name is what was provided via the query string paramter socket. That should be the necessary information to transfer the same over to prisma-engines.

@brian-dlee
Copy link
Author

Yep, that's right.

I'm happy to make the same code changes to prisma-engines if you think that's necessary here. I'm not exactly an expert in Rust. My skill level is moderate, though. I've written a handful of small apps.

@janpio
Copy link
Member

janpio commented May 21, 2024

Not necessary, but the right thing for consistency - as you raised yourself about the error message,, that would otherwise be misleading.

@brian-dlee brian-dlee force-pushed the feat/display-socket-during-migrate-reset branch from 5e6dc9b to ee93ebd Compare May 28, 2024 17:09
Copy link

codspeed-hq bot commented May 28, 2024

CodSpeed Performance Report

Merging #24222 will improve performances by 21.08%

Comparing brian-dlee:feat/display-socket-during-migrate-reset (ee93ebd) with main (29a309f)

Summary

⚡ 1 improvements
✅ 2 untouched benchmarks

Benchmarks breakdown

Benchmark main brian-dlee:feat/display-socket-during-migrate-reset Change
client generation 100 models with relations 7.5 s 6.2 s +21.08%

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.

None yet

3 participants