fix(handler): group captured log messages by call site, not message content - #164
Conversation
posthog-elixir Compliance ReportDate: 2026-07-09 19:59:27 UTC ✅ All Tests Passed!46/46 tests passed Capture Tests✅ 29/29 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
|
That's an interesting topic! To my knowledge, there's no clear cut solution to tracking log messages which can potentially contain dynamic values. It's always a bit of a trade off between undergrouping and overgrouping. Using call site for fingerprinting will work great for very broad messages like Initially I decided against using call site for fingerprinting because I though undergrouping is easier to addres:
My understanding is that overgrouping would be much harder to address:
WDYT? |
|
thanks for looking @martosaur! yeah, I think it's a bit tricky because we've seen (recently at least) a lot of I was thinking I wonder if there's also the idea of having a |
|
oooh, if you have access to the aggregate data, is there a way to check if those unique fingerprints have any large clusters? The thing is, most people I know learn fairly quickly to not interpolate dynamic values in logs, but there is a couple of very popular libraries that are guilty of that. For example I would expect |
|
I think we should avoid putting the message on the exception type, imo the grouping decision should be handled server side and behavior between SDKs should be aligned. If users want to control grouping on the SDK they can still rely on the I also agree that overgrouping should be avoided, we released a new fingerprint algorithm that better take the exception value into account (and strip dynamic fields) when there is no stacktrace. It's not bullet proof but that should help as the message is the only thing we can use to split two error inside a single function. |
|
Reviews (1): Last reviewed commit: "fix(handler): keep report text for trans..." | Re-trigger Greptile |
dustinbyrne
left a comment
There was a problem hiding this comment.
Human-driven, agent-assisted review.
| meta: %{mfa: {module, function, arity}, domain: [:elixir | _]} = meta | ||
| }) | ||
| when not is_map_key(meta, :__posthog_crash_reporter__), | ||
| do: "Logger #{level} (#{Exception.format_mfa(module, function, arity)})" |
There was a problem hiding this comment.
[P2/design] Should this group by enclosing function rather than the individual logging statement?
The generated type identifies the enclosing MFA, so two different statements such as:
def run(id) do
Logger.error("Bad request #{id}")
Logger.error("Upstream timeout #{id}")
endboth emit:
Logger error (MyApp.Worker.run/1)
Current fingerprinting omits the exception message for resolved stacks and uses the resolved source/module/function without the line number. I confirmed with a focused V2 test that different messages and line numbers in the same function produce the same fingerprint.
Is that function-level grouping intentional?
There was a problem hiding this comment.
Yes, it should be fine until we release a new fingerprint algo based on source context. It's not ideal but we cannot include the lines in the fingerprint as it can shift from one release to another. Also, tbh I think we should focus on exceptions and not logs, grouping strings and stacktraces is really different and we have another dedicated product for that 🤓
There was a problem hiding this comment.
I think the real answer is using crash_reason when you want a proper handled exception, which seems like what mainstream libraries already set as well, I was looking a bit through what & how is being used. we handle that case correctly and use the exception/stacktrace, i.e. Logger.error(..., crash_reason: {e, __STACKTRACE__}) shouldn't be affected by this grouping, I'll open a docs PR to mention this.
for example, Logger.Translator already does this and Bandit, Plug.Cowboy etc (but I agree with @martosaur that a lot of libraries still don't do this properly)
plus, maybe we should have a capture_exception/1 dedicated method as well for explicit captures
💡 Motivation and Context
Exception events built from plain log messages (
capture_levelcaptures without acrash_reason) currently use the formatted message as the exceptiontype. Log messages routinely interpolate dynamic values — ids, URLs, inspected terms — so every distinct message produces a distinct type. Error tracking fingerprints the exception type verbatim, which means a singleLogger.error("Failed to crawl #{url}")call site creates a separate issue per event instead of one issue for the call site. In production data this makes captured-log exception events group dramatically worse than crash-derived ones: the large majority of such events end up with a unique fingerprint, and server-side masking can't help because the volatile parts are often plain words (URLs, entity names), not just numbers.This PR keys the type on the logging call site instead:
Logger error (MyApp.Crawler.update_stats/2), derived from themfalog metadata (the same metadata #152 uses for the synthetic stacktrace frame). The full message is still sent invalue, so nothing is lost from the issue description or event detail. Whenmfametadata is absent (e.g.Logger.bare_log/3), behavior is unchanged.The crash-reporter entry of chained exceptions (a log message with an attached
crash_reason) intentionally keeps its message-derived type: that entry names the issue, and for e.g. Bandit/Cowboy request crashes the banner (** (RuntimeError) oops) is a much better issue title than the logging call site inside the HTTP server would be. The marker that gates this never leaves the handler.💚 How did you test it?
mix test(316 passed),mix format,mix credo --strict(no new findings).📝 Checklist
If releasing new changes
sampo addto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
$exception_fingerprint(bypasses server grouping rules), and applying the call-site type to crash-reporter chain entries (would regress issue titles for request crashes).typefor message captures, so existing issues for such events will re-key once — those groups are the ones currently splitting per event, so the practical cost is nil.