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: correct clause in SSL closed message and handle GenServer logger crash #49

Merged
merged 1 commit into from
Jan 18, 2025

Conversation

mrdotb
Copy link
Owner

@mrdotb mrdotb commented Jan 16, 2025

  • Fixes a clause error in the SSL closed message introduced in ssl error presence #45
  • Addresses a GenServer crash that was breaking the logger handler

Contributor checklist

  • My commit messages follow the Conventional Commit Message Format
    For example: fix: Multiply by appropriate coefficient, or
    feat(Calculator): Correctly preserve history
    Any explanation or long form information in your commit message should be
    in a separate paragraph, separated by a blank line from the primary message
  • Bug fixes include regression tests

@mrdotb mrdotb requested a review from martosaur January 16, 2025 13:39
@mrdotb mrdotb self-assigned this Jan 16, 2025
Copy link
Collaborator

@martosaur martosaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch on the Transport error incorrect tuple!

For the failing logger handler, I reproduced this scenario and this is the actual stacktrace:

19:22:04.850 [debug] [logger: :removed_failing_handler, handler: {:"test with a crashing GenServer GenServer crash should not crash the logger handler", DiscoLog.LoggerHandler}, 
  log_event: %{
    meta: %{
      error_logger: %{
        tag: :error,
        report_cb: &:gen_server.format_log/1
      },
      line: 2646,
      pid: #PID<0.255.0>,
      time: 1737084124836572,
      file: ~c"gen_server.erl",
      gl: #PID<0.69.0>,
      domain: [:otp],
      mfa: {:gen_server, :error_info, 8},
      report_cb: &:gen_server.format_log/2,
      crash_reason: {{:error, nil, %Mint.TransportError{reason: :closed}, []}, []}
    },
    msg: {:string, [
      ["GenServer ", "{DiscoLog.Registry, DiscoLog.Presence}", " terminating",
       ["\n** (stop) " | "{:error, nil, %Mint.TransportError{reason: :closed}, []}"], 
       [], "\nLast message", [], ": ", "{:ssl, :fake_ssl_closed}"],
      "\nState: ",
      "%DiscoLog.Presence{
         discord: DiscoLog.DiscordMock,
         discord_config: %DiscoLog.Discord.Config{
           category: %{name: \"disco-log\", type: 4},
           tags: [\"plug\", \"live_view\", \"oban\"],
           occurrences_channel: %{
             name: \"occurrences\",
             type: 15,
             available_tags: [%{name: \"plug\"}, %{name: \"live_view\"}, %{name: \"oban\"}]
           },
           info_channel: %{name: \"info\", type: 0},
           error_channel: %{name: \"error\", type: 0},
           token: \"mytoken\",
           guild_id: \"guild_id\",
           category_id: \"category_id\", 
           occurrences_channel_id: \"occurences_channel_id\",
           occurrences_channel_tags: %{},
           info_channel_id: \"info_channel_id\",
           error_channel_id: \"error_channel_id\",
           enable_discord_log: false
         },
         presence_status: \"🪩 Disco Logging\",
         registry: nil,
         websocket_client: %DiscoLog.WebsocketClient{
           conn: nil,
           websocket: %Mint.WebSocket{
             extensions: [],
             fragment: nil,
             private: %{},
             buffer: \"\"
           },
           ref: nil,
           state: :closed
         },
         jitter: 0.7958348364490507,
         heartbeat_interval: nil,
         sequence_number: nil,
         waiting_for_ack?: nil
       }"
    ]},
    level: :error
  },
  config: %{
    id: :"test with a crashing GenServer GenServer crash should not crash the logger handler",
    module: DiscoLog.LoggerHandler,
    config: %{
      metadata: [],
      enable: true,
      token: "mytoken",
      otp_app: :foo,
      guild_id: "guild_id",
      category_id: "category_id",
      occurrences_channel_id: "occurences_channel_id", 
      info_channel_id: "info_channel_id",
      error_channel_id: "error_channel_id",
      discord: DiscoLog.DiscordMock,
      enable_presence: true,
      repo_url: "",
      discord_config: %DiscoLog.Discord.Config{
        category: %{name: "disco-log", type: 4},
        tags: ["plug", "live_view", "oban"],
        occurrences_channel: %{
          name: "occurrences",
          type: 15,
          available_tags: [%{name: "plug"}, %{name: "live_view"}, %{name: "oban"}]
        },
        info_channel: %{name: "info", type: 0},
        error_channel: %{name: "error", type: 0},
        token: "mytoken",
        guild_id: "guild_id",
        category_id: "category_id",
        occurrences_channel_id: "occurences_channel_id",
        occurrences_channel_tags: %{},
        info_channel_id: "info_channel_id",
        error_channel_id: "error_channel_id",
        enable_discord_log: false
      },
      presence_status: "🪩 Disco Logging",
      supervisor_name: DiscoLog,
      enable_logger: true,
      instrument_phoenix: true,
      instrument_oban: true,
      instrument_tesla: true,
      enable_discord_log: false,
      excluded_domains: [:cowboy, :bandit],
      before_send: nil,
      enable_go_to_repo: false,
      go_to_repo_top_modules: [],
      git_sha: ""
    },
    formatter: {:logger_formatter, %{}}
  },
  reason: {:error, :function_clause, [
    {DiscoLog.Error, :normalize_exception, [{:error, nil, %Mint.TransportError{reason: :closed}, []}, []], [file: ~c"lib/disco_log/error.ex", line: 58]},
    {DiscoLog.Error, :new, 4, [file: ~c"lib/disco_log/error.ex", line: 23]},
    {DiscoLog.LoggerHandler, :log_from_crash_reason, 4, [file: ~c"lib/disco_log/logger_handler.ex", line: 174]}
  ]}
]

I think for a solid fix it would be nice to allow DiscoLog.Error to work with any reason, since it looks like it can be any arbitrary value from the genserver exit? Doesn't have to be in this PR of course

@mrdotb
Copy link
Owner Author

mrdotb commented Jan 17, 2025

👋
Could you share how you trigger the error from test GenServer ?
In my current test I pass the error manually to the Logger because I could not reproduce the error from the test GenServer.

I think for a solid fix it would be nice to allow DiscoLog.Error to work with any reason, since it looks like it can be any arbitrary value from the genserver exit?

The genserver exit are covered.
In this case It seems that there 2 error wrapped if the way I test it is correct because I could not reproduce it with GenServer test.

%{
  meta: %{
    line: 416,
    pid: #PID<0.247.0>,
    time: 1737107615148176,
    file: ~c"/home/john/Projects/elixir/disco-log/test/disco_log/logger_handler_test.exs",
    gl: #PID<0.70.0>,
    domain: [:elixir],
    mfa: {DiscoLog.LoggerHandlerTest,
     :"test with a crashing GenServer GenServer crash should not crash the logger handler",
     1}
  },
  msg: {:report,
   %{
     meta: %{
       domain: [:otp],
       mfa: {:gen_server, :error_info, 8},
       report_cb: &:gen_server.format_log/2,
       crash_reason: {{:error,
         %Mint.HTTP1{
           host: nil,
           port: nil,
           request: nil,
           streaming_request: nil,
           socket: nil,
           transport: nil,
           mode: nil,
           scheme_as_string: nil,
           case_sensitive_headers: nil,
           requests: {[], []},
           state: :closed,
           buffer: "",
           proxy_headers: [],
           private: %{},
           log: false
         }, %Mint.TransportError{reason: :closed}, []}, []}
     },
     msg: {:string,
      [
        ["GenServer ", "{DiscoLog.Registry, DiscoLog.Presence}", " terminating"],
        ["\n** (stop) ",
         "{:error, %Mint.HTTP1{host: \"gateway.discord.gg\", port: 443, request: nil, streaming_request: nil, ... }"],
        "\nLast message: ",
        "{:ssl_closed, {:sslsocket, {:gen_tcp, 12345, :tls_connection, :undefined}, [self(), self()]}}",
        "\nState: ",
        "%DiscoLog.Presence{discord: DiscoLog.Discord, ... }"
      ]},
     level: :error
   }},
  level: :error
}

@martosaur
Copy link
Collaborator

Sure, here's a branch with the test https://github.com/mrdotb/disco-log/tree/am/fix/presence-ssl-error

The error and logger failure message matches what I see in my app when this happens.

… crash

- Fixes a clause error in the SSL closed message introduced with presence
- Addresses a GenServer crash that was breaking the logger handler
@mrdotb mrdotb force-pushed the fix/presence-ssl-error branch from 33bd8c7 to ca97009 Compare January 18, 2025 16:05
@mrdotb
Copy link
Owner Author

mrdotb commented Jan 18, 2025

👍 Thanks for the help.
I found where the logger break with your test setup and fixed it.

@mrdotb mrdotb closed this Jan 18, 2025
@mrdotb mrdotb reopened this Jan 18, 2025
@mrdotb mrdotb merged commit c0a4052 into main Jan 18, 2025
2 checks passed
@mrdotb mrdotb deleted the fix/presence-ssl-error branch January 26, 2025 10:48
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.

2 participants