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

Implement SQL commenter as a Sequelize wrapper #2

Merged
merged 2 commits into from
Jan 4, 2024
Merged

Conversation

eliseier
Copy link
Collaborator

@eliseier eliseier commented Jan 3, 2024

Motivation

Notion

The original google sqlcommenter Sequelize plugin included a lot of dependencies that we didn't need since we just wanted trace context (e.g. dependencies on opencensus, opentelemetry).

This forks the plugin and creates a minimal version.

Changes

  1. Add makeMinimalStacktrace which gives us good context regarding a Sequelize query stacktrace.
  2. Change wrapSequelize to auto-wrap the query.run method with a stacktrace comment

Note: We're essentially reaching in and changing private functions (query.run). TypeScript doesn't allow us to do this due to better enforced privacy, so it is required that we do so with a JS plugin instead of implementing the solution outright in our codebase with TS.

Testing

  1. Added unit tests & ran with mocha
  2. Logged the SQL queries to ensure it looks how we want it to.

@eliseier eliseier changed the title Implement SQL commenter as a Sequelize wrapper #1 Implement SQL commenter as a Sequelize wrapper Jan 3, 2024
package.json Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@eliseier
Copy link
Collaborator Author

eliseier commented Jan 3, 2024

as a general note, i did find this article on warning against injecting dynamic information as comments for SQL queries since it ruins the cache:
https://www.adyen.com/knowledge-hub/injecting-metadata-as-comments

we might see some CPU usage go up since now, different origins of similar queries may result in separate cache entries; however, the stack information is generally deterministic

potential follow-ups:

  • make this a configurable option that we can only turn on sometimes for debugging

Copy link

@pxpeterxu pxpeterxu left a comment

Choose a reason for hiding this comment

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

This mostly looks good! Some small suggestions about where to put the escaping code, etc.

On your prepared statement note:

as a general note, i did find this article on warning against injecting dynamic information as comments for SQL queries since it ruins the cache: https://www.adyen.com/knowledge-hub/injecting-metadata-as-comments

we might see some CPU usage go up since now, different origins of similar queries may result in separate cache entries; however, the stack information is generally deterministic

potential follow-ups:

* make this a configurable option that we can only turn on sometimes for debugging

I took a look, and I think we don't need to worry about this quite yet -- https://www.adyen.com/knowledge-hub/injecting-metadata-as-comments seems to talk mostly about prepared statements using the PREPARE commands thanks to the Java library they use.

For Sequelize with MySQL, I don't believe Sequelize uses prepared statements; a quick search through the codebase seems to indicate that the only appearance of prepare is in the IBM DB2 driver, and MySQL likely just uses plain queries with substitutions, so we should be safe for now!

index.js Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
util.js Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
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.

2 participants