Include params and arity in formatted backtrace #115

sorentwo · 2017-11-13T15:38:17Z

This enhances the backtrace to include function arity and arguments, if available. The arguments are inspected to ensure they can be serialized as JSON without any issues.

Argument inclusion is configured with filter_args, which defaults to false. Therefore by default arguments are included in a notice's backtrace. When arguments are filtered an empty list will be returned instead.

Also note, Poison can't encode tuples, structs, pids, refs, etc. so inspect is called on each argument. It may be preferable to be more selective with argument formatting, as a struct or list may be quite large. Any thoughts on this?

Addresses #78

minhajuddin

This looks great 👍

minhajuddin · 2017-11-19T07:38:38Z

README.md

-| api_key      | Your application's Honeybadger API key                                    | System.get_env("HONEYBADGER_API_KEY"))` |
-| environment_name | (required) The name of the environment your app is running in.                   | nil |
-| app          | Name of your app's OTP Application as an atom                             | Mix.Project.config[:app] |
-| use_logger   | Enable the Honeybadger Logger for handling errors outside of web requests | true |


I think you accidentally removed app and use_logger documentation

You're correct, I did! I've restored it.

minhajuddin · 2017-11-19T12:45:11Z

lib/honeybadger/backtrace.ex

+  defp format_args(args, false = _filter) when is_list(args) do
+    Enum.map(args, &inspect/1)
+  end
+  defp format_args(_arity, _filter) do


Can we rename this to _args

When it isn't a list it is a number, which really is the arity of the function. Stack traces are guaranteed to have either the args or arity, so this is indicating what the argument really is.

My bad :) I thought the first argument would always be a list. I thought, this head was to match when filter was true. 👍

minhajuddin · 2017-11-19T12:51:24Z

mix.exs

@@ -36,6 +36,7 @@ defmodule Honeybadger.Mixfile do
           notice_filter: Honeybadger.NoticeFilter.Default,
           filter: Honeybadger.Filter.Default,
           filter_keys: [:password, :credit_card],
+           filter_args: false,


If this is an opt-in feature, we should default to filter_args: true. @joshuap thoughts?

Yeah, I'm thinking we want to disable this feature by default. We discussed making it opt-in in #78 (comment). Does that make sense, @sorentwo?

I've reversed the default, it is now true, making it opt-in like we discussed 👍

joshuap · 2017-11-20T15:56:15Z

Also note, Poison can't encode tuples, structs, pids, refs, etc. so inspect is called on each argument. It may be preferable to be more selective with argument formatting, as a struct or list may be quite large. Any thoughts on this?

I think we do want to do something to handle super large structures. In our ruby gem we truncate the maximum string size that can be sent as any value to 64KiB. That's more of a catch-all which applies to any data (including context). We also apply some extra sanitization to #inspect output by default -- for instance, #<MyObject id=1 foo=bar ... > becomes #<MyObject>. We could do something similar here, although I'm not sure what makes sense yet. The easiest thing to do would probably be to come up with some standard representation of un-encodable objects.

This enhances the backtrace to include function arity and arguments, if available. The arguments are inspected to ensure they can be serialized as JSON without any issues.

sorentwo · 2017-11-21T15:20:05Z

@joshuap I've started on a spike to implement customized to_string handling with a protocol. It will provide conservative defaults for all Elixir types and we or other users can add new implementations to extend it if needed.

Going with %MyObject{} over %MyObject{id: 1, foo: :bar} makes a lot of sense to me, so I've started that direction.

minhajuddin · 2017-11-25T08:21:00Z

@sorentwo Is there anything pending on this PR? Are you planning on pushing a new commit with the review suggestion changes?

sorentwo · 2017-11-25T13:54:59Z

Yes, there is work pending and a commit that I haven’t pushed. I’ve been on vacation for the past week, this will be a little while longer.

To prevent serializing entire structs or lengthy binaries we now pass opts to `Kernel.inspect/2` when generating a backtrace.

sorentwo · 2017-11-28T02:17:59Z

@minhajuddin @joshuap The changes to inspected arguments have been pushed up. We're using Kernel.inspect/2 with some custom options. This handles every possible structure, and with limit and printable_limit we get protection against large lists, structs, and binaries.

I've set the list/tuple/struct limit to 5 elements, and the printed limit to 1024 bytes. Those felt like reasonable amounts, let me know what you think. The values are all configured in a module attribute at the top of Honeybadger.Backtrace, and would be easy to modify.

minhajuddin · 2017-11-28T09:06:17Z

Looks great 👍

sorentwo · 2017-12-07T21:38:00Z

Is anything blocking this still? I'd love to get it merged, it would be helpful with a few errors I'm seeing currently.

joshuap · 2017-12-07T21:52:49Z

@sorentwo we still need to implement on the backend, sorry for the delay. I'll look into that next.

joshuap · 2017-12-08T01:46:18Z

@sorentwo I'm trying to test this out in my Phoenix app. I'm getting the function arity, but I'm still getting empty args when I set the config filter_args: false. Any ideas what I'm doing wrong?

 elixir --version
Erlang/OTP 20 [erts-9.1.5] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:10] [hipe] [kernel-poll:false] [dtrace]

Elixir 1.5.2

sorentwo · 2017-12-08T02:27:19Z

@joshuap According to this post on erlang questions by Lukas Larsson (@garazdawi) the only time it will ever give arguments instead of arity is:

You get that information if the error is a function clause, however if you crash for any other reason (badmatch, badarith, etc) within a function you cannot get that information.

The reason for this is because the compiler may have found that the arguments to the function in which you crashed to be dead and optimized the values away.

I've been experimenting with causing various errors and checking the stacktrace, I only seem to be able to get the arity. I'm not sure what else to do here, I'll try looking at how elixir/iex manages to display arguments and clauses so thoroughly.

sorentwo · 2017-12-08T21:18:19Z

Very relevant to this discussion, and the future handling of stacktraces: elixir-lang/elixir#7097

joshuap · 2017-12-13T00:42:57Z

I created issue #123 to track improvements to argument handling. In any case, Honeybadger now supports displaying arguments with the stack trace, so I'm going to merge this!

Edit: Here's a screenshot of a stack trace w/ arity: https://screenshots.firefox.com/QvQDSbBPKao2c0qG/app.honeybadger.io

minhajuddin suggested changes Nov 19, 2017

View reviewed changes

Include params and arity in formatted backtrace

cbdd843

This enhances the backtrace to include function arity and arguments, if available. The arguments are inspected to ensure they can be serialized as JSON without any issues.

Apply limits when inspecting arguments

67b716c

To prevent serializing entire structs or lengthy binaries we now pass opts to `Kernel.inspect/2` when generating a backtrace.

minhajuddin approved these changes Nov 28, 2017

View reviewed changes

joshuap self-assigned this Dec 12, 2017

Merge branch 'master' into feature/backtrace-params

b67c006

joshuap mentioned this pull request Dec 13, 2017

Improve argument reporting #123

Open

joshuap merged commit d113e4c into honeybadger-io:master Dec 13, 2017

sorentwo deleted the feature/backtrace-params branch January 5, 2018 19:58

joshuap mentioned this pull request Jul 7, 2021

Can't get filter_args: false to work #368

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include params and arity in formatted backtrace #115

Include params and arity in formatted backtrace #115

sorentwo commented Nov 13, 2017

minhajuddin left a comment

minhajuddin Nov 19, 2017

sorentwo Nov 21, 2017

minhajuddin Nov 19, 2017

sorentwo Nov 21, 2017

minhajuddin Nov 21, 2017 •

edited

Loading

minhajuddin Nov 19, 2017

joshuap Nov 20, 2017

sorentwo Nov 21, 2017

joshuap commented Nov 20, 2017

sorentwo commented Nov 21, 2017

minhajuddin commented Nov 25, 2017

sorentwo commented Nov 25, 2017

sorentwo commented Nov 28, 2017

minhajuddin commented Nov 28, 2017

sorentwo commented Dec 7, 2017

joshuap commented Dec 7, 2017

joshuap commented Dec 8, 2017 •

edited

Loading

sorentwo commented Dec 8, 2017

sorentwo commented Dec 8, 2017

joshuap commented Dec 13, 2017 •

edited

Loading

Include params and arity in formatted backtrace #115

Include params and arity in formatted backtrace #115

Conversation

sorentwo commented Nov 13, 2017

minhajuddin left a comment

Choose a reason for hiding this comment

minhajuddin Nov 19, 2017

Choose a reason for hiding this comment

sorentwo Nov 21, 2017

Choose a reason for hiding this comment

minhajuddin Nov 19, 2017

Choose a reason for hiding this comment

sorentwo Nov 21, 2017

Choose a reason for hiding this comment

minhajuddin Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

minhajuddin Nov 19, 2017

Choose a reason for hiding this comment

joshuap Nov 20, 2017

Choose a reason for hiding this comment

sorentwo Nov 21, 2017

Choose a reason for hiding this comment

joshuap commented Nov 20, 2017

sorentwo commented Nov 21, 2017

minhajuddin commented Nov 25, 2017

sorentwo commented Nov 25, 2017

sorentwo commented Nov 28, 2017

minhajuddin commented Nov 28, 2017

sorentwo commented Dec 7, 2017

joshuap commented Dec 7, 2017

joshuap commented Dec 8, 2017 • edited Loading

sorentwo commented Dec 8, 2017

sorentwo commented Dec 8, 2017

joshuap commented Dec 13, 2017 • edited Loading

minhajuddin Nov 21, 2017 •

edited

Loading

joshuap commented Dec 8, 2017 •

edited

Loading

joshuap commented Dec 13, 2017 •

edited

Loading