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 :crash_reason metadata handling #533

Closed
wants to merge 2 commits into from
Closed

Conversation

pojiro
Copy link

@pojiro pojiro commented Jan 30, 2023

  • mix format
  • mix test

Use case

This PR fixes following use case with config :logger, Sentry.LoggerBackend, metadata: [:crash_reason].

  def test_with_exception() do
    try do
      raise RuntimeError
    rescue
      exception ->
        Logger.error("sentry test with exception", crash_reason: {exception, __STACKTRACE__})
    end
  end

Currently we cannot log for other backends and Sentry.LoggerBackend at the same time with one line.
So we need to write like following,

  def test_with_exception() do
    try do
      raise RuntimeError
    rescue
      exception ->
        Logger.error("sentry test with exception") # ex. for console
        Sentry.capture_exception(error, stacktrace: __STACKTRACE__)
    end
  end

Why cannot currently

The following code in the current master branch cannot work because it fails to encode JSON.
Because the :crash_reason value is a tuple.

{%_{__exception__: true} = exception, stacktrace} when is_list(stacktrace) ->
Sentry.capture_exception(exception, [stacktrace: stacktrace] ++ opts)

:crash_reason is special metadata, so I think we don't need to use it for :logger_metadata in :extra.

@pojiro pojiro marked this pull request as ready for review January 30, 2023 13:15
@pojiro
Copy link
Author

pojiro commented Jan 31, 2023

@mitchellhenke Thank you for your great work!! Could you review this PR?

@sl0thentr0py
Copy link
Member

@pojiro thx for the PR, standalone this PR is weird since people could just not add it to the :metadata config for LoggerBackend.
But if we merge #523 in, this makes sense (with maybe some other defaults excluded as well), so I'll decide what to do on this once we resolve that other PR.

@pojiro
Copy link
Author

pojiro commented Feb 6, 2023

@sl0thentr0py thx for your reply and following comment.

standalone this PR is weird since people could just not add it to the :metadata config for LoggerBackend.

I fixed my 1st comment to explain the purpose of this PR.
I will try to re-create a PR again as I may be able to resolve #523 and this with one PR. Thanks.

@pojiro
Copy link
Author

pojiro commented Feb 7, 2023

I tried, but could not satisfy :all. The reason is that there is no way to determine which keys in meta are or are not serializable.

here is the example of meta from, test "includes Logger.metadata for keys configured to be included"
[
  erl_level: :error,
  crash_reason: {%FunctionClauseError{
     args: nil,
     arity: 3,
     clauses: nil,
     function: :from_erl,
     kind: nil,
     module: NaiveDateTime
   },
   [
     {NaiveDateTime, :from_erl, [{}, {}, {}],
      [file: 'lib/calendar/naive_datetime.ex', line: 811]},
     {Sentry.TestGenServer, :handle_info, 2,
      [file: 'test/support/test_gen_server.exs', line: 56]},
     {:gen_server, :try_dispatch, 4, [file: 'gen_server.erl', line: 695]},
     {:gen_server, :handle_msg, 6, [file: 'gen_server.erl', line: 771]},
     {:proc_lib, :init_p_do_apply, 3, [file: 'proc_lib.erl', line: 226]}
   ]},
  domain: [:otp],
  error_logger: %{report_cb: &:gen_server.format_log/1, tag: :error},
  file: "gen_server.erl",
  function: "error_info/7",
  gl: #PID<0.64.0>,
  line: 949,
  list: [1, 2, 3],
  map: %{a: "b"},
  mfa: {:gen_server, :error_info, 7},
  module: :gen_server,
  number: 43,
  pid: #PID<0.399.0>,
  report_cb: &:gen_server.format_log/2,
  string: "string",
  time: 1675730928476391
]

@sl0thentr0py
Copy link
Member

I'll need to think about this too, not that familiar with elixir. But we need some kind of general serializable? abstraction I guess.

@pojiro
Copy link
Author

pojiro commented Feb 10, 2023

It might be. It would be nice to be able to make changes in a direction that is consistent with the current source, but I just can't take the time to do to that.

@whatyouhide
Copy link
Collaborator

This is obsolete now that we merged #605. Thank you @pojiro! 💟

@whatyouhide whatyouhide closed this Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants