Conversation
First step towards using Errata for error handling. It covers one flow end to end, broadcast replay, so the pattern is established and reviewable before committing to it. Realtime.Messages.replay/5 now returns ReplayRejected (bad params, public channel) or ReplayFailed (query failure, wrapping whatever the tenant Repo returned) instead of bare atoms. Each error records the tenant, topic, since, and limit that were requested, plus where it was created. The channel join reports any Errata error through a new Logging.log_error/2, which logs it under the error's code and attaches the type, reason, context, cause and origin as Logger metadata. The metadata is nested under an `error` key because the Logflare backend overwrites a top-level `context`. Client-facing reasons and the UnableToReplayMessages error code are unchanged, so dashboards, alerts and existing tests are unaffected. One behavior change: a query that returns a Postgrex error, rather than raising, was previously reported as UnknownErrorOnChannel and now reaches the client as a replay failure.
CRAP Score Report |
|
Coverage is 91.602% — chore/introduce-errata into main. No base build found for |
| """ | ||
| use Errata.InfrastructureError, | ||
| default_message: "Realtime was unable to replay messages", | ||
| code: "UnableToReplayMessages" |
There was a problem hiding this comment.
I notice these both have the same code and neither matches the module name. With error modules as specific to a single path/feature as these are, do we end up with hundreds of error modules and is that a problem? Do we need to plan out a taxonomy of errors to avoid that?
There was a problem hiding this comment.
I certainly hope we won't have that many error modules, but planning out the error taxonomy is an important part of integrating Errata into the codebase.
The code is intended for external error reporters and aggregators, and I think it would be fairly common for multiple Errata errors to share a code so that they can be aggregated together.
Does that answer your questions?
| backend overwrites top-level `context`, `level` and `stacktrace` metadata with its own. | ||
| """ | ||
| @spec log_error(socket :: Phoenix.Socket.t(), error :: Errata.error()) :: {:error, %{reason: binary}} | ||
| def log_error(socket, error) when is_error(error) do |
There was a problem hiding this comment.
Do we need to think about log sanitisation here? Since we're logging more context it could be easy for someone to accidentally log something that shouldn't be logged.
There was a problem hiding this comment.
Good call out. Errata supports redaction of sensitive data both at the individual error level and at global config level, but I hadn't configured it yet. In commit dddcda3 I added the following global config:
config :errata,
redact: [
# credentials clients send
:access_token,
:user_token,
:auth_token,
:apikey,
"x-api-key",
:authorization,
# tenant secrets
:jwt_secret,
:db_password,
:password
]
| assert :erpc.call(node, Messages, :replay, [pid, tenant.external_id, "test", 0, 30]) == | ||
| {:error, :failed_to_replay_messages} | ||
| # The error is created on the remote node and travels back as a struct, context and all | ||
| assert {:error, %ReplayFailed{} = error} = |
There was a problem hiding this comment.
What's the impact on the wire if we're sending more than an atom back as an error message from an rpc call? And is that worth it if in most cases we're just logging the error?
There was a problem hiding this comment.
It will certainly increase the amount of data sent over the wire, but only when errors are being passed around nodes. I wouldn't think this would be happening frequently enough to be of concern, although I'm willing to be corrected on that.
Sensitive data will now be redacted when errors propagate to logs or any other external error reporting system.
This branch has not been deployed
3 participants
First step towards using Errata for error handling. It covers one flow end to end, broadcast replay, so the pattern is established and reviewable before committing to it.
Realtime.Messages.replay/5now returnsReplayRejected(bad params, public channel) orReplayFailed(query failure, wrapping whatever the tenant Repo returned) instead of bare atoms. Each error records the tenant, topic, since, and limit that were requested, plus where it was created. The channel join reports any Errata error through a newLogging.log_error/2, which logs it under the error's code and attaches the type, reason, context, cause and origin as Logger metadata. The metadata is nested under anerrorkey because the Logflare backend overwrites a top-levelcontext.Client-facing reasons and the "UnableToReplayMessages" error code are unchanged, so dashboards, alerts and existing tests are unaffected. One behavior change: a query that returns a Postgrex error, rather than raising, was previously reported as "UnknownErrorOnChannel" and now reaches the client as a replay failure.
What kind of change does this PR introduce?
A first, deliberately small step towards structured error handling with Errata. This PR covers one flow end to end: broadcast replay on channel join.
What is the current behavior?
Replay failures come out of
Realtime.Messages.replay/5as bare atoms. The channel join maps each one to a fixed string in its own clause, and the resulting log event knows only the tenant:Which topic, which replay window, and which line produced the error are not recorded. A query that returns a Postgrex error, rather than raising, falls through to
UnknownErrorOnChannel.What is the new behavior?
replay/5returns one of two Errata errors:ReplayRejectedfor bad params or a public channel, andReplayFailedfor a query failure, wrapping whatever the Repo returned. Both carry the requested tenant, topic, since and limit, and the point where they were created.The join handles any Errata error with a single clause. A new
Logging.log_error/2logs it under the error's code and attaches the rest as Logger metadata, nested undererrorbecause the Logflare backend overwrites a top-levelcontext. Same event as above:Client-facing reasons and the
UnableToReplayMessagescode are unchanged, so dashboards, alerts and the existing channel tests are unaffected. The one behaviour change is that a returned Postgrex error is now reported as a replay failure instead of an unknown error.Additional context
Replay was picked because it is self-contained: one origin, one boundary, and it crosses the RPC hop, which shows the error struct travelling between nodes intact.
The
causeabove is a bare atom becauseRealtime.Tenants.Reporescues the real exception and returns:postgrex_exception. Having Repo return the exception itself is the natural follow-up, as is the HTTP side, where the fallback controller can render straight fromErrata.http_status/1.Needs errata 1.9.2: earlier versions defined a global
Jason.EncoderforTuplethat clashed with the one inRealtime.Logs.Disclosure: I'm the author of Errata.