Add current stacktrace log #363
Conversation
|
@tashirosota this looks nice, thanks for pinging! How would you configure it in your app? |
integration_test/sql/logging.exs
Outdated
| before_log_level = Logger.level() | ||
|
|
||
| try do | ||
| Logger.configure(level: :debug) | ||
| assert ExUnit.CaptureLog.capture_log(fn -> | ||
| TestRepo.insert!(%Post{title: "1"}, [log: true]) | ||
| end) =~ "[debug]" | ||
|
|
||
| refute ExUnit.CaptureLog.capture_log(fn -> | ||
| TestRepo.insert!(%Post{title: "1"}, [log: true]) | ||
| end) =~ " ↳ " | ||
| after | ||
| Logger.configure(level: before_log_level) | ||
| end | ||
|
|
There was a problem hiding this comment.
Not sure if this really has an impact, but logger level is for the whole application, so changing it through Log.configure may affect other tests and may also be flaky (as this test is async)
There was a problem hiding this comment.
@v0idpwn
Thanks for review！
I understood. But I don't know how to test it other than that.
Please give me a hint🙇♂️
lib/ecto/adapters/sql.ex
Outdated
| @@ -983,6 +995,12 @@ defmodule Ecto.Adapters.SQL do | |||
| ?\s, | |||
| inspect(params, charlists: false) | |||
| ] | |||
| if stacktrace_match_path = Application.get_env(:ecto_sql, :log_stacktrace_match_path) do | |||
There was a problem hiding this comment.
This should be stored in the opts, as opposed to application env. This is a better practice for configurability.
There was a problem hiding this comment.
Also, I'm wondering if there's a better approach instead of matching the first result into selected path. Is there any other option you've considered?
There was a problem hiding this comment.
@v0idpwn
Thanks for review.
This should be stored in the opts, as opposed to application env. This is a better practice for configurability.
I changed it to log_stacktrace_match_path = Keyword.get(opts, :log_stacktrace_match_path) || Keyword.get(opts, :otp_app) |> to_string() and this logging to default output. Is it bad to have the default output?
Also, I'm wondering if there's a better approach instead of matching the first result into selected path. Is there any other option you've considered?
Because I referenced to the Rails implementation. And it's often useful.
Or output with all match stack trace? But I think that will became noisy log.
|
@josevalim @v0idpwn |
tashirosota
changed the title
tashirosota
force-pushed
the
feature/log_current_stacktrace
branch
4 times, most recently
from
f5f8768
to
4664159
Compare
| @@ -60,6 +60,30 @@ defmodule Ecto.Adapters.SQL do | |||
| * `:driver` (required) - the database driver library. | |||
| For example: `:postgrex` | |||
|
|
|||
| ## Debug log | |||
|
|
|||
| You can get caller location when SQL called and log level is :debug. | |||
There was a problem hiding this comment.
@josevalim
I want to this log is default debug output, and get stacktrace location from String.match?(/otp_app_name/).
But may not work well, so users can set log_stacktrace_match_path and get it from String.match?(/log_stacktrace_match_path/).
tashirosota
changed the title
|
Hi @tashirosota! Thanks for the work so far, I am discussing with @v0idpwn the best way to provide this, which may require some changes in Ecto too. |
tashirosota
force-pushed
the
feature/log_current_stacktrace
branch
from
4664159
to
e5bbfc2
Compare
|
Hey, @tashirosota! I opened a PR where I tried to move the get stacktrace part to ecto, and only turn it into a log in ecto_sql. My goal is to avoid the traces of ecto insides so we don't need to process the stacktrace. |
|
@v0idpwn |
Hi! Special thanks for ecto_sql and contributers.
AR has powerfull debug log output when sql called. I like it.
I implemented it to ecto_sql. If you don't mind check it.
How about incorporating this feature?