refactor: Redesign main modules to enable ad-hoc usage

[martosaur]

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
• Features include unit/acceptance tests

This one is big conceptually, but in terms of lines count, it is a net negative. About 25% of added lines are canned API responses (more on this below).

Goal

My original goal here was to enable an ad-hoc usage of DiscoLog. Suppose you don't use DiscoLog as a logger handler, but still want to manually publish messages to certain channels. This makes sense because a medium-sized application can generate too many logs to actually post into discord. But posting some logs (for example, a message that a new user signed up to a paid plan) into some channels is useful.

In this scenario, you're likely to not have a valid config on your hands and virtually all parts of DiscoLog required it. In fact, there were two types of configs: DiscoLog.Config and DiscoLog.Discord.Config.

Hopefully, this refactoring will make things simpler.

Changes

DiscoLog.Discord module was removed

The problem with Discord module was that it served as a watershed between two parts of the system, both of which were important. However, because Discord was used for mocking, the logic behind it was untested.

Win: the tests now able to reach behind Discord module all the way to HTTP requests and test more.

DiscoLog.Discord.Context and DiscoLog.Discord.Client were dissolved

Discord.Context and Discord.Client did many things: they served as modules making HTTP requests for other modules as well as preparing messages and errors for submitting to the API.

Having 2 pair of modules named Context and Client is a little confusing, so I split their responsibilities around. Mainly between DiscoLog.Discord.API and DiscoLog.Prepare

New module DiscoLog.Prepare

The logic about how to represent errors and log messages as discord messages lives here. This module is akin to what is usually called Formatters, but I chose a different name here to avoid confusion with logger handler formatters. This module consists of only pure functions which makes it convenient to test.

New module DiscoLog.Discord.API

This is a Discord API client inspired by Req's take on SDKs. It is a behaviour, but instead of defining every endpoint as a callback, it defines only 2: client/1 and request/4. This should be more than enough to allow anyone who would want to switch to another HTTP client to do so.

Besides callbacks ,DiscoLog.Discord.API also includes functions for API calls used by the application, such as list_active_threads and post_thread. Note that endpoints only used by mix commands are not included as functions. Instead, mix commands issue ad-hoc requests.

API modules come with default implementation API.Client. For tests, there is also API.Mock and API.Stub. Stub modules contains a collection of response examples that will be returned by default. In my experience, having canned responses is very convenient for development.

Win: This change makes HTTP client fully replaceable. In fact, we can make Req dependency optional now. This also allows users to call discord endpoints not used by DiscoLog, e.g.:

DiscoLog.Discord.API.Client.client(config.token).client |> DiscoLog.Discord.API.Client.request(:get, "/guilds/#{config.guild_id}/emojis", [])

Changes to DiscoLog.Config

before_send is removed. I don't think it was used, and I think the equivalent functionality can be achieved via logger filters.

discord option was changed to discord_client_module and is now DiscoLog.Discord.API.Client by default.

During validation, Config will now call discord_client_module.client(token) and save it as discord_client for everyone to use.

DiscoLog.Client is merged into DiscoLog

DiscoLog.Client is an interesting one. It had a number of fairly generic send_error, log_info and log_error functions, which for me look very similar to DiscoLog.report, so I moved them to DiscoLog

DiscoLog.Discord.Config is removed

After removing the watershed which Discord was, there is no need in discord config anymore.

Changes to DiscoLog.LoggerHandler

I removed adding_handler and changing_config callbacks. They are used for fairly advanced use cases, which are probably beyond DiscoLog intended applications.

Result

There are undoubtedly some rough edges worth further investigating and work, but I'm happy to report that ad-hoc usage is unblocked. Here's how I can post a message to arbitrary channel with just token and channel_id:

DiscoLog.log_info("Hello, world!", %{}, %{discord_client: DiscoLog.Discord.API.Client.client(token), info_channel_id: "1302395538922147954"})

[mrdotb]

👋 A lot of changes so I am not sure I saw everything.

I'am not a fan of breaking the discord context and spreading logic around (Storage, Mix task ...)
I prefer to have all the api calls in API even if they are not all used in production like the mix tasks. From my experience it's easier to maintain / extend the api this way.

For the API call that require a bit of twist or glue like fetch_or_create, maybe_*, get_occurrence_threads I prefer them to live in a separate module Context like before or to be grouped at least cause they are quite complicated and they should expose the most simple function to the consumer like {:ok, data} {:error, reason} without having to know how it works.

Then should they be a Sub module of Discord is a choice to make.

[mrdotb]

The title aka name where not tested. It's a good idea to test it adding a public helper that use a internal module would be nicer if we happen to change the fingerprint.

[mrdotb]

The title aka name where not tested. It's a good idea to test it adding a public helper that use a internal module would be nicer if we happen to change the fingerprint.

[mrdotb]

I'm a bit confused about those changes.
This test are important because it's all the case that detach the logger handler
Also the return and how it's formatted after passing by the handler is not tested anymore.

[martosaur]

The idea was to only test integration here, and leave detailed assertions of error shape to tests in other modules, but in retrospect, that doesn't seem like too much hassle. I added the assertions back!

[martosaur]

I'am not a fan of breaking the discord context and spreading logic around (Storage, Mix task ...)
I prefer to have all the api calls in API even if they are not all used in production like the mix tasks. From my experience it's easier to maintain / extend the api this way.

For the API call that require a bit of twist or glue like fetch_or_create, maybe_*, get_occurrence_threads I prefer them to live in a separate module Context like before <..._

Sure, we can move them back into a single place. You have any thoughts on the name? I'd argue that having two Context module is a little confusing. Maybe use the DiscoLog.Discord that is free now?

they should expose the most simple function to the consumer like {:ok, data} {:error, reason} without having to know how it works.

One problem I usually run into with modules like that is that they inevitably swallow errors when they return error tuple. The caller is usually the one who knows what to do in case of an error, and losing a detailed error can make it harder to handle different scenarios or uncover bugs. There's a huge difference between get_occurrence_threads failing due to random 502 gateway timeout or receiving a completely unexpected response shape that DiscoLog didn't account for. In the app it can be somewhat mitigated with logs, but for library code I think the common expectation is to not log unless completely necessary. WDYT?

[mrdotb]

Sure, we can move them back into a single place. You have any thoughts on the name? I'd argue that having two Context module is a little confusing. Maybe use the DiscoLog.Discord that is free now?

DiscoLog.Discord seems good now that it's available 👍

One problem I usually run into with modules like that is that they inevitably swallow errors when they return error tuple. The caller is usually the one who knows what to do in case of an error, and losing a detailed error can make it harder to handle different scenarios or uncover bugs. There's a huge difference between get_occurrence_threads failing due to random 502 gateway timeout or receiving a completely unexpected response shape that DiscoLog didn't account for. In the app it can be somewhat mitigated with logs, but for library code I think the common expectation is to not log unless completely necessary. WDYT?

While I've also encountered issues with swallowed errors, for the Discord integration specifically, I see it this way:

• For transient issues (network errors, gateway timeouts), we're already well-covered by Req's built-in retry mechanism (https://hexdocs.pm/req/Req.Steps.html#retry/1). This handles most temporary failures automatically and gracefully.
• For API shape mismatches (when Discord changes their API), this is more of a debugging scenario than an error handling one. In these cases, you'll need visibility into the raw response to understand what changed, which is best handled by enabling Discord's debug mode rather than through error returns.
• For configuration issues (invalid tokens, insufficient permissions), these should fail fast during runtime - there's little benefit in trying to handle these gracefully since they indicate fundamental setup problems. Those cases could throw an error since we want the user to notice it and it can't work unless it's fixed. ex 401 / 403 status

I don't want the caller to decide what's a success and what is an error and what error should be handled. I prefere those to be handled in the called module and the api call will probably have shared logic like 400 case.

          with {:ok, %{status: 201, body: %{"id" => thread_id}}} <-
                 API.post_thread(config.discord_client, config.occurrences_channel_id, message) do
            Storage.add_thread_id(config.supervisor_name, error.fingerprint, thread_id)
            # added stuff for illustration purpose
            else
              {:error, %Req.SomeNetworkError} ->
                 # transient
              %{:ok, %{status: 401, body: _} ->
                 # throw or do something
               other_ ->
                swallow ?
          end

In the app it can be somewhat mitigated with logs, but for library code I think the common expectation is to not log unless completely necessary. WDYT?

I agree no log from a library unless configured.

I noticed you removed the configurable logger for discord could you bring it back it's very usefull for dev.

    |> maybe_add_debug_log(config.enable_discord_log)
  end

  defp maybe_add_debug_log(request, false), do: request

  defp maybe_add_debug_log(request, true) do
    Req.Request.append_response_steps(request, log_response: &log_response/1)
  end

  defp log_response({req, res} = result) do
    Logger.debug("""
      Request: #{inspect(to_string(req.url), pretty: true)}
      Request: #{inspect(to_string(req.body), pretty: true)}
      Status: #{inspect(res.status, pretty: true)}
      Body: #{inspect(res.body, pretty: true)}
    """)

    result
  end

[martosaur]

I pushed a commit where I created a DiscoLog.Discord module and moved some previously private functions with API calls to it. Please check and let me know if you like the direction

[mrdotb]

I like the direction where it's going 🚀
I notice a small issue but it's still wip.

[mrdotb]

If one channel already exist the pattern matching will fail.
fetch_or_create_* should return {:ok, channel}
Low chance to happen because they will be all created or none most of the time.

[martosaur]

Ah, right, fixed that!

[mrdotb]

A successful creation should return a 201 status and a channel and should be unwrapped to {:ok, channel}

[martosaur]

I don't want the caller to decide what's a success and what is an error and what error should be handled. I prefere those to be handled in the called module and the api call will probably have shared logic like 400 case.

They can be handled by the called module, sure, but ultimately only the caller can decide what to do. Take 400 error for example. If Storage process encountered a 400 while getting existing occurrences threads, it wants to crash right away as it can't work. If logger handler encountered a 400, it wants to perhaps log and carry on, because failing is not an option for the handler. So both callers require enough information from the response to make this decision and thus relying on us choosing the right granularity of errors in the wrapper module. And it's just hard to get right. But if we ditch wrapper module all together, it's the best granularity available.

Then there's the topic of error message. In two scenarios above, we want to ideally log as much information as possible and in many cases, HTTP response is the best thing available. Enabling a debug mode with full request logging is definitely possible and good, but you will have to hope that the same error will happen again and that it is indeed the same error. Not to mention debug logs tend to be noisy and hunting down errors can be tricky.

If you hide 401 behind {:error, :authorization} error and crash disco log application, it's a clear signal that something is misconfigured, but it's not enough info to figure out if it's a typo in bot token, or if user misconfigured the bot somehow, or something else. Sure, debug will help here, but why the extra steps of enabling it if the error happened here and now?

It's just hard to see the good side of a wrapper module I think? Maintainability usually means that we can write something once an reuse in other places, but every single function in Discord.Context module is used in just one place outside of it. There's just not a lot of code duplication just yet IMO.

Anyways, I won't insist on doing it my way, just wanted to ensure all arguments are laid out clearly.

[mrdotb]

I get your point about error granularity and letting the caller decide how to handle things like 400 errors. But I still think the context module has value for what we consider a successful call. For example, it helps keep Storage focused and not have to worry about API details, like mapping data from responses or extracting fingerprints.

For functions where we want access to the error body, what do you think about this pattern and letting the raw error pass through?

with {status: 201, body: %{"stuff" => stuff}} <- Api.create_stuff(attrs) do
   map_stuff(stuff)
end

The API calls that don’t have data mapping don’t need to be in the wrapper/context.

[martosaur]

Wouldn't it be more like this with some errors being also wrapped in :ok tuple like {:ok, %{status: 400 ...}}?

with {:ok, %{status: 201, body: %{"stuff" => stuff}}} <- Api.create_stuff(attrs) do
   {:ok, map_stuff(stuff)}
end

that would probably need to be something like

case API.create_stuff(stuff) do
  {:ok, %{status: 201, body: %{"stuff" => stuff}}} -> {:ok, map_stuff(stuff)}
  {:ok, resp} -> {:error, resp}
  error -> error
end

I think this is how it would naturally evolve once map_stuff is sufficiently sophisticated and used in multiple places, but there's probably no harm in doing it rather sooner than later. Does that version look right to you?

[mrdotb]

Y I like the idea of letter the raw error pass while mapping the success call if needed

[mrdotb]

👋 Looks good to merge

[martosaur]

Added back debug logs and squashed commits!

[mrdotb]

🚀