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: Accept queryLabel query option for logging. #14787

Merged
merged 5 commits into from Jul 22, 2022

Conversation

sandinmyjoints
Copy link
Contributor

@sandinmyjoints sandinmyjoints commented Jul 22, 2022

Pull Request Checklist

Please make sure to review and check all of these items:

  • Have you added new tests to prevent regressions?
  • Does yarn test or yarn test-DIALECT pass with this change (including linting)?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description Of Change

Accept queryLabel as query option (open to alternate names, like loggingLabel or just label). Extremely useful for labeling queries when query logging is turned on. For example, when I have query logging turned on and I hit an endpoint in one of my applications, a dozen or more queries may show up in the logs. This allows me to provide a name/label so I can quickly jump to the code that produced a given query.

Todos

@sandinmyjoints sandinmyjoints force-pushed the feature-named-queries branch 5 times, most recently from 3883fc3 to fcbd534 Compare July 22, 2022 17:29
@sandinmyjoints sandinmyjoints changed the title Accept queryName query option for logging. feat: Accept queryName query option for logging. Jul 22, 2022
@sandinmyjoints sandinmyjoints marked this pull request as ready for review July 22, 2022 18:20
Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

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

Seems like a nice feature to add. Could you add a test for when the query has been executed as well (the afterMsg). And maybe we should add something for when queryName is '' so that you don't get an additional empty character at the start.

@sandinmyjoints sandinmyjoints force-pushed the feature-named-queries branch 2 times, most recently from fdd9f90 to 54f7318 Compare July 22, 2022 19:16
@sandinmyjoints sandinmyjoints changed the title feat: Accept queryName query option for logging. feat: Accept queryLabel query option for logging. Jul 22, 2022
Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com>
@WikiRik
Copy link
Member

WikiRik commented Jul 22, 2022

Thanks for this PR!

About the documentation, you can open a PR on the website repo to add it to the logging section; https://github.com/sequelize/website/blob/main/docs/getting-started.mdx#logging but since it's also mentioned in the types it should be reflected in the API reference as well.
I would however appreciate it if it's added to the constructor docs, just like the other options.

@sandinmyjoints
Copy link
Contributor Author

sandinmyjoints commented Jul 22, 2022

@WikiRik Updated the docstring. Thanks for the quick review!

src/sequelize.js Outdated Show resolved Hide resolved
sandinmyjoints and others added 2 commits July 22, 2022 17:06
Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com>
Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com>
@WikiRik WikiRik merged commit d6854c7 into sequelize:main Jul 22, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2022

🎉 This PR is included in version 7.0.0-alpha.15 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

2 participants