feat(alerts): format notification values and breakdown labels - #67981
Merged
jakesciotto merged 4 commits intoJul 3, 2026
Conversation
Insight-alert notifications (Slack and email) rendered raw internal values: the breakdown sentinel `$$_posthog_breakdown_other_$$` and unformatted floats like `803.7740196999998`, with no currency symbol, decimal rounding, or the insight's configured prefix/postfix. The insight's display config (`trendsFilter.aggregationAxisFormat/Prefix/Postfix/decimalPlaces` plus `team.base_currency`) is already available in `TrendsExtractor.extract()` but was never consulted when building the breach message. Wire it in via a new `formatting.py` (a backend port of the frontend `formatAggregationAxisValue`) and an opt-in `value_formatter` on `ExtractionResult` that the comparator applies. Breakdown labels are humanized to their display strings. Both Slack and email consume the same `breaches` strings, so both channels improve with no HogFunction/template change. The PERCENTAGE-threshold path (relative-change ratios) short-circuits before the formatter, so those messages are unchanged; funnels and SQL insights set no formatter and are unaffected. Generated-By: PostHog Code Task-Id: 8dba907d-3504-4958-b316-a1752e4c63f9
Contributor
|
Reviews (1): Last reviewed commit: "feat(alerts): format notification values..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+67
to
+74
| def test_make_trends_value_formatter_reads_filter_and_currency() -> None: | ||
| fmt = make_trends_value_formatter(TrendsFilter(aggregationAxisFormat=A.CURRENCY), "USD") | ||
| assert fmt(803.7740196999998) == "$803.77" | ||
|
|
||
|
|
||
| def test_make_trends_value_formatter_handles_none_filter() -> None: | ||
| fmt = make_trends_value_formatter(None, None) | ||
| assert fmt(803.7740196999998) == "803.77" |
Contributor
There was a problem hiding this comment.
make_trends_value_formatter can be collapsed into a single @pytest.mark.parametrize block — the team's stated preference. Both have the same shape: construct a formatter from (filter, currency), call it with a value, assert the result string.
Suggested change
| def test_make_trends_value_formatter_reads_filter_and_currency() -> None: | |
| fmt = make_trends_value_formatter(TrendsFilter(aggregationAxisFormat=A.CURRENCY), "USD") | |
| assert fmt(803.7740196999998) == "$803.77" | |
| def test_make_trends_value_formatter_handles_none_filter() -> None: | |
| fmt = make_trends_value_formatter(None, None) | |
| assert fmt(803.7740196999998) == "803.77" | |
| @pytest.mark.parametrize( | |
| "trends_filter,currency,expected", | |
| [ | |
| (TrendsFilter(aggregationAxisFormat=A.CURRENCY), "USD", "$803.77"), | |
| (None, None, "803.77"), | |
| ], | |
| ) | |
| def test_make_trends_value_formatter( | |
| trends_filter: TrendsFilter | None, currency: str | None, expected: str | |
| ) -> None: | |
| fmt = make_trends_value_formatter(trends_filter, currency) | |
| assert fmt(803.7740196999998) == expected |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
mypy flagged the defensive `if not isinstance(label, str)` branch as unreachable since the parameter is typed `str`. The callers always pass a string, so the guard is dead code — remove it. Generated-By: PostHog Code Task-Id: 8dba907d-3504-4958-b316-a1752e4c63f9
Contributor
|
Reviews (2): Last reviewed commit: "fix(alerts): drop unreachable isinstance..." | Re-trigger Greptile |
Generated-By: PostHog Code Task-Id: 8dba907d-3504-4958-b316-a1752e4c63f9
MattPua
requested changes
Jul 2, 2026
MattPua
left a comment
MattPua
left a comment
Member
There was a problem hiding this comment.
there's some parts that we've already implemented for Max AI that can be used again here.
reusing that would help reduce the manual copy pasta for the formatting file, and make this slimmer.
Also would be good if @vdekrijger was able to give a quick glance since he's done a lot of work on alerting recently
| } | ||
|
|
||
|
|
||
| def humanize_breakdown_label(label: str) -> str: |
Member
There was a problem hiding this comment.
the robot is telling me that replace_breakdown_labels already exists which basically does the same thing
Comment on lines
+167
to
+207
| def _format_currency(value: float, currency: str | None) -> str: | ||
| """Prefix the humanized number (fixed 2 decimals, like the frontend formatCurrency) with the | ||
| currency symbol. Unknown currencies fall back to the code; a missing currency falls back to "$".""" | ||
| number = _human_friendly_number(value, DEFAULT_DECIMAL_PLACES, DEFAULT_DECIMAL_PLACES) | ||
| if not currency: | ||
| return f"${number}" | ||
| symbol = _CURRENCY_SYMBOLS.get(currency.upper()) | ||
| if symbol is not None: | ||
| return f"{symbol}{number}" | ||
| return f"{currency.upper()} {number}" | ||
|
|
||
|
|
||
| def _format_duration_seconds(value: float, *, seconds_fixed: int | None = None) -> str: | ||
| """Convert seconds to a human-readable duration ("45s", "2m 12s", "1h 4m", "850ms"). | ||
|
|
||
| Port of the frontend humanFriendlyDuration; handles negative (relative differences) and zero. | ||
| """ | ||
| if value < 0: | ||
| return f"-{_format_duration_seconds(-value, seconds_fixed=seconds_fixed)}" | ||
| if value == 0: | ||
| return "0s" | ||
| if value < 1: | ||
| return f"{round(value * 1000)}ms" | ||
| if value < 60: | ||
| return f"{_trim_float(f'{value:.{seconds_fixed or 0}f}')}s" | ||
|
|
||
| days = math.floor(value / 86400) | ||
| hours = math.floor((value % 86400) / 3600) | ||
| minutes = math.floor((value % 3600) / 60) | ||
| seconds = math.floor((value % 3600) % 60) | ||
|
|
||
| day_display = f"{days}d" if days > 0 else "" | ||
| hour_display = f"{hours}h" if hours > 0 else "" | ||
| minute_display = f"{minutes}m" if minutes > 0 else "" | ||
| second_display = f"{seconds}s" if seconds > 0 else ("" if (hour_display or minute_display) else "0s") | ||
|
|
||
| if days > 0: | ||
| units = [u for u in (day_display, hour_display) if u] | ||
| else: | ||
| units = [u for u in (hour_display, minute_display, second_display) if u] | ||
| return " ".join(units) |
Member
There was a problem hiding this comment.
robot also says we have some of this existing in python ee folder already
Contributor
Author
There was a problem hiding this comment.
used robot to re-use whatever it knew was there
The new alerts formatting.py added `humanize_breakdown_label`, a byte-for-byte duplicate of `replace_breakdown_labels` in ee/hogai — both wrapping the same `.replace()` over the sentinel constants from `insights/utils/breakdowns.py`. Hoist a single `humanize_breakdown_label` into that breakdowns module, next to the sentinel constants and display strings it maps, and point both the alerts trends extractor and the hogai trends formatter at it. Drop the now-unused constant imports from both former call sites. Consolidate the two redundant unit tests (one in alerts, one in ee/hogai) into one canonical parameterized test alongside the function in test_breakdowns.py. No behavior change: the humanizer logic is identical, only relocated. Generated-By: PostHog Code Task-Id: 2c5b7b95-2d5b-4f70-8d76-dc1ae7b7c237
MattPua
approved these changes
Jul 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Insight-alert notifications (Slack and email) render raw internal values instead of what the insight itself shows on the chart. A firing alert reads:
Two problems: the internal breakdown sentinel
$$_posthog_breakdown_other_$$leaks into the message, and values are raw floats with no currency symbol, decimal rounding, thousands separators, or the insight's configured prefix/postfix.Changes
The insight's display config (
trendsFilter.aggregationAxisFormat, prefix/postfix, decimal places) andteam.base_currencyare already available inTrendsExtractor.extract(). They just weren't consulted when building the breach message. This wires them in:formatting.pyin the alerts evaluation package: a backend port of the frontendformatAggregationAxisValue. Handles currency, duration, percentage, short, prefix/postfix and decimals, plus ahumanize_breakdown_labelthat swaps the breakdown sentinels for their display strings. Reusescompact_numberfromposthog.utils.ExtractionResultgets an opt-invalue_formatterfield (defaultsNone).TrendsExtractorbuilds it from the trends filter plus base currency, and the comparator applies it to both the breach value and the threshold bound.TrendsExtractor._to_series, which also cleans up the persistedtriggered_metadatarow labels.Same message now reads:
Slack and email both consume the same
breachesstrings, so both channels improve with no HogFunction or Slack-template change.Two behavior-preserving guards: the PERCENTAGE-threshold path (relative-change ratios) short-circuits before the formatter, so those messages are byte-for-byte unchanged, and funnels and SQL insights set no formatter so they fall through to the existing rendering.
Decisions
babel. It sits behind a single_format_currencyseam, so swapping in locale-correct formatting later is a one-function change. Unknown codes fall back to aCODEprefix, a missing currency falls back to$.803.7740196999998becomes803.77,10.0becomes10). This is the source of the integration-test literal churn below and is the intended side effect.How did you test this code?
I (Claude) wrote and ran what the sandbox allowed:
test_formatting.py(parameterized, pure): one case per format branch plus the edges that could regress in production - the raw-float trim, currency fallback on an unknown or missing code, negative and zero durations (relative differences can be negative),trendsFilter=None, and each breakdown-label shape (standalone, action-prefixed,::-joined, and a normal label containing-that must survive untouched). Ran directly: 28/28 pass.test_comparator.pycases: the formatter is applied to the value and both bounds (catches dropping the wiring), and a PERCENTAGE threshold ignores a supplied formatter (locks the short-circuit so a future refactor can't corrupt relative-% messages).test_trends_absolute_alerts.py, the actual regression this fixes, which no existing test covered.X.0becomesX; the%andinf%percentage-threshold literals are unchanged).Ran locally and green:
ruff check+ruff format,tach check --dependencies --interfaces,py_compileon all changed modules, and the 28-case formatter matrix.I could not run the Django or ClickHouse backed suites (the comparator and the three trends integration files) locally - this sandbox has no Postgres or ClickHouse and Docker is unavailable. They run in CI. I hand-traced the two comparator scenarios and verified every reconciled literal follows the single humanization rule.
Automatic notifications
Docs update
No docs under
docs/describe the alert message format, so there's nothing to update. The change is a readability improvement to notification output.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Jake directed this from a screenshot of the raw Slack output and the observation that the formatting data should already be available. I (Claude Code, Opus 4.8) explored the alert evaluation pipeline, confirmed the config was reachable at check time, and implemented the wiring. The
/writing-testsskill was invoked before adding tests to keep the new cases pointed at real regressions.Notable choices while building: I kept
value_formatteropt-in onExtractionResultso the existing comparator unit tests, funnels, and SQL alerts stay untouched - only the trends path sets it. I considered gating currency behindbabelbut chose the no-dependency symbol map to keep the change self-contained, leaving a clean seam to upgrade later. The scope decision (format all trend alerts vs only currency-configured ones) was deliberate: formatting everywhere is what fixes the raw-float case generally, at the cost of updating a batch of integration-test literals.