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

fix: query and error logging both occur on error. #796

Merged
merged 4 commits into from
Dec 29, 2023

Conversation

igalklebanov
Copy link
Member

@igalklebanov igalklebanov commented Dec 4, 2023

closes #743.

This is better than the suggested solution in the comment section. If we moved query logging to the try body, we'd catch errors in user's logging logic. User has all the context necessary to handle his logic's exceptions and print necessary errors.

Regarding timing mismatch, not much we can do about it.

Went ahead and fixed defaultLogger's error handling when not Error - it was printing [object Object].

@igalklebanov igalklebanov added the bug Something isn't working label Dec 4, 2023
Copy link

vercel bot commented Dec 4, 2023

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

Name Status Preview Comments Updated (UTC)
kysely ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 29, 2023 10:00am

@igalklebanov igalklebanov changed the title fix: query and error logging occurs on error. fix: query and error logging both occur on error. Dec 4, 2023
@igalklebanov igalklebanov marked this pull request as ready for review December 4, 2023 23:25
@igalklebanov igalklebanov requested a review from koskimas December 4, 2023 23:25
Copy link
Member

@koskimas koskimas left a comment

Choose a reason for hiding this comment

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

LGTM

Awesome tests as usual. In fact maybe too awesome 😄 We should try to write the minimum amount of tests that still do the job "well enough". We also need to maintain the tests, and having a gazillion tests just slows down development.

@koskimas koskimas force-pushed the fix-double-logging-on-error branch from 7795366 to 7bdd416 Compare December 29, 2023 10:00
@koskimas koskimas merged commit 87d1041 into kysely-org:master Dec 29, 2023
5 checks passed
@igalklebanov igalklebanov deleted the fix-double-logging-on-error branch December 29, 2023 11:47
thecodrr pushed a commit to thecodrr/kysely that referenced this pull request Sep 3, 2024
* deny logging query and error onerror. also deny catching custom log err.

* move caughtError declaration so it doesn't take up "execution time".

* fix defaultLogger error handling when not Error.

* test logging.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Different query duration in error and query events
2 participants