Allow configuring stacktrace for logging #658
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A lot of times, more stack items can be helpful in identifying the source of a query. This commit demonstrates how we can support it. But the API has problems: 1. the `:stacktrace` option is used not only by logging but also for general telemetry events. Making it an integer doesn't make sense for telemetry (as the full stacktrace is published there anyway). 2. the `:stacktrace` option is overriden Ecto-side, so Ecto.SQL can't accept integers on a per-query basis, only on global configuration.
Left the alternative `last_non_ecto` implementation taking a parameter for length. Goal is to make it easier to just increase the length without needing to implement a new function or use a 3rd party package.
v0idpwn
changed the title
v0idpwn
force-pushed
the
feat/stacktrace-depth
branch
from
6ee7ee5 to
80e5eb8
Compare
v0idpwn
commented
Mar 28, 2025
josevalim
reviewed
Mar 29, 2025
josevalim
reviewed
Mar 29, 2025
josevalim
reviewed
Mar 29, 2025
josevalim
reviewed
Mar 29, 2025
Co-authored-by: José Valim <[email protected]>
|
@josevalim Pushed all your suggestions, thanks for the thorough review! Only one thing: EctoSQL integration CI is running on v1.11.4 (this is the oldest supported version as well). Edit: I decided to just bump CI elixir version :) |
v0idpwn
force-pushed
the
feat/stacktrace-depth
branch
from
07ecdf5 to
f833e9a
Compare
Ecto already requires ~> 1.14
josevalim
reviewed
Mar 29, 2025
| @@ -1377,21 +1425,29 @@ defmodule Ecto.Adapters.SQL do | |||
|
|
|||
| @repo_modules [Ecto.Repo.Queryable, Ecto.Repo.Schema, Ecto.Repo.Transaction] | |||
|
|
|||
| defp last_non_ecto([{mod, _, _, _} | _stacktrace], repo, last) | |||
| def first_non_ecto_stacktrace(stacktrace, repo, size) do | |||
There was a problem hiding this comment.
We need to document this function. But also document the new option in Ecto. But this function could have a full example of how to configure it and use it :)
josevalim
approved these changes
Mar 29, 2025
v0idpwn
force-pushed
the
feat/stacktrace-depth
branch
from
0ea86d4 to
da99c79
Compare
josevalim
reviewed
Mar 31, 2025
| with [_ | _] <- stacktrace, | ||
| {module, function, arity, info} <- last_non_ecto(Enum.reverse(stacktrace), repo, nil) do | ||
| defp log_stacktrace([_ | _] = stacktrace, repo, {module, function, args}) do | ||
| entries = apply(module, function, [stacktrace, repo | args]) |
There was a problem hiding this comment.
Suggested change
| entries = apply(module, function, [stacktrace, repo | args]) | |
| entries = apply(module, function, [stacktrace, %{repo: repo} | args]) |
As per the Ecto issue.
josevalim
approved these changes
Mar 31, 2025
Comment on lines
+1438
to
+1440
| |> Enum.reverse() | ||
| |> last_non_ecto_entries(repo, []) | ||
| |> Enum.take(size) |
There was a problem hiding this comment.
I'm not sure how big these stacktraces can get. Is it better to pass a decrementing counter to last_non_ecto_entries and reverse at the end? Or it doesn't really matter?
Co-authored-by: Greg Rychlewski <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Click to expand old description
A lot of times, more stack items can be helpful in identifying the source of a query.This PR demonstrates how we can support it. But the API has problems:
the
:stacktraceoption is used not only by logging but also for general telemetry events. Making it an integer doesn't make sense for telemetry (as the full stacktrace is published there anyway).the
:stacktraceoption is overriden Ecto-side, so Ecto.SQL can't accept integers on a per-query basis, only on global configuration.Adds a new option
:log_stacktrace_mfathat can be used at configuration level or query level (like:log, or:stacktrace). This config allows filtering/prepping the stacktrace-derived data displayed in the logs. For example, one may want to filter other modules other than those that Ecto.SQL filters by default, or maybe just include more stack items.Sample usage: