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

Logging SQL #939

Closed
andrewthad opened this Issue Feb 20, 2015 · 10 comments

Comments

Projects
None yet
3 participants
@andrewthad
Contributor

andrewthad commented Feb 20, 2015

With the scaffolded yesod site, the log output for SQL queries looks like this:

20/Feb/2015:11:18:22 -0500 [Debug#SQL] "SELECT \"draft_sales_order_line\".\"id\", \"draft_sales_order_line\".\"draft_id\", ...

I have truncated the line because it is very long. However, my point is that is it a Haskell-escaped string. This would be great if I was planning on sticking it in GHCi to replay the query, but I never do that. Moreover, I feel pretty confident that almost no one is doing that. I open up psql and copy the queries into that (after having to run a script to unescape C-style strings). It would be nice if the logs just printed the actual query. I'd like to see either one of two things:

  1. Persistent's logging behavior being changed (I think this would be trivial).
  2. The same thing but with optional buy-in.
    The line causing this behavior is right here: https://hackage.haskell.org/package/persistent-2.1.1.4/docs/src/Database-Persist-Sql-Raw.html#rawSql
    In the definition of rawQueryRes where we call show sql. Thanks for any feedback or consideration of this proposal.
@MaxGabriel

This comment has been minimized.

Show comment
Hide comment
@MaxGabriel

MaxGabriel Feb 20, 2015

Member

Printing the actual query sounds better. I've often generated some SQL with an ORM and then pasted it into a MySQL/Postgres console, especially to run EXPLAIN on the query.

Member

MaxGabriel commented Feb 20, 2015

Printing the actual query sounds better. I've often generated some SQL with an ORM and then pasted it into a MySQL/Postgres console, especially to run EXPLAIN on the query.

snoyberg added a commit to yesodweb/persistent that referenced this issue Feb 22, 2015

Slightly nicer SQL logging yesodweb/yesod#939
Avoids escaping the SQL code for easier reading
@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Feb 22, 2015

Member

I've just pushed a commit to the persistent repo to address this. I haven't tested it myself in the scaffolding, but it should work as expected. @andrewthad if you can give this a go and let me know if it addresses your concern, I can release to Hackage.

Member

snoyberg commented Feb 22, 2015

I've just pushed a commit to the persistent repo to address this. I haven't tested it myself in the scaffolding, but it should work as expected. @andrewthad if you can give this a go and let me know if it addresses your concern, I can release to Hackage.

@andrewthad

This comment has been minimized.

Show comment
Hide comment
@andrewthad

andrewthad Feb 23, 2015

Contributor

I don't have an easy way to run it right now, but from looking at your commit I can tell that it does what I want. If you're ok with releasing it without me running it, then you could go ahead with that, but if you'd rather have someone check it, then I can do that that sometime tomorrow.

Contributor

andrewthad commented Feb 23, 2015

I don't have an easy way to run it right now, but from looking at your commit I can tell that it does what I want. If you're ok with releasing it without me running it, then you could go ahead with that, but if you'd rather have someone check it, then I can do that that sometime tomorrow.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Feb 24, 2015

Member

I'm fine waiting another day or two before releasing.

Member

snoyberg commented Feb 24, 2015

I'm fine waiting another day or two before releasing.

@andrewthad

This comment has been minimized.

Show comment
Hide comment
@andrewthad

andrewthad Feb 24, 2015

Contributor

I just tried it out and it works. The only other thing I'll mention (because I just noticed this) is that the bit at the end that shows the source file containing the code that called the logger seems like it's sort of a waste, especially given that (1) all sql logs originate here and (2) None of the other server logs that yesod creates have source file information. But I'm content with it as is if that's not something you want to do. Here is the output I get from your a scaffolded site with your latest commit:

24/Feb/2015:11:26:34 -0500 [Debug#SQL] SELECT "id","ident","password" FROM "user" WHERE "ident"=?; [PersistText "andrew.thaddeus@gmail.com"] @(persistent-2.1.1.5:Database.Persist.Sql.Raw ./Database/Persist/Sql/Raw.hs:42:26)
Contributor

andrewthad commented Feb 24, 2015

I just tried it out and it works. The only other thing I'll mention (because I just noticed this) is that the bit at the end that shows the source file containing the code that called the logger seems like it's sort of a waste, especially given that (1) all sql logs originate here and (2) None of the other server logs that yesod creates have source file information. But I'm content with it as is if that's not something you want to do. Here is the output I get from your a scaffolded site with your latest commit:

24/Feb/2015:11:26:34 -0500 [Debug#SQL] SELECT "id","ident","password" FROM "user" WHERE "ident"=?; [PersistText "andrew.thaddeus@gmail.com"] @(persistent-2.1.1.5:Database.Persist.Sql.Raw ./Database/Persist/Sql/Raw.hs:42:26)
@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Feb 24, 2015

Member

Changing that location thing would be more invasive than it would seem, and would actually break a very useful feature: try using $logError in your app itself, and you'll see why it's present. Yes, in the case of library-generated data, it's useless.

I'll release this to Hackage now, thanks for checking it!

Member

snoyberg commented Feb 24, 2015

Changing that location thing would be more invasive than it would seem, and would actually break a very useful feature: try using $logError in your app itself, and you'll see why it's present. Yes, in the case of library-generated data, it's useless.

I'll release this to Hackage now, thanks for checking it!

@snoyberg snoyberg closed this Feb 24, 2015

@andrewthad

This comment has been minimized.

Show comment
Hide comment
@andrewthad

andrewthad Feb 24, 2015

Contributor

I wasn't suggesting that we redefine $logDebugS, only that we change the code in persistent to use logDebugN instead. But, it can wait for some other time. Not my most crucial concern.

Contributor

andrewthad commented Feb 24, 2015

I wasn't suggesting that we redefine $logDebugS, only that we change the code in persistent to use logDebugN instead. But, it can wait for some other time. Not my most crucial concern.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Feb 24, 2015

Member

Oh, that's a good idea, I hadn't thought of doing so. Let me test that out
now...

On Tue Feb 24 2015 at 6:45:24 PM andrewthad notifications@github.com
wrote:

I wasn't suggesting that we redefine $logDebugS, only that we change the
code in persistent to use logDebugN instead. But, it can wait for some
other time. Not my most crucial concern.


Reply to this email directly or view it on GitHub
#939 (comment).

Member

snoyberg commented Feb 24, 2015

Oh, that's a good idea, I hadn't thought of doing so. Let me test that out
now...

On Tue Feb 24 2015 at 6:45:24 PM andrewthad notifications@github.com
wrote:

I wasn't suggesting that we redefine $logDebugS, only that we change the
code in persistent to use logDebugN instead. But, it can wait for some
other time. Not my most crucial concern.


Reply to this email directly or view it on GitHub
#939 (comment).

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Feb 24, 2015

Member

Done :)

Member

snoyberg commented Feb 24, 2015

Done :)

@andrewthad

This comment has been minimized.

Show comment
Hide comment
@andrewthad

andrewthad Feb 24, 2015

Contributor

Sweet. Thanks for getting that in.

Contributor

andrewthad commented Feb 24, 2015

Sweet. Thanks for getting that in.

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