Repository navigation
Fill gaps from assistant conversations: SMS/email OTP and Prometheus percentiles - #449
mintlify[bot] wants to merge 5 commits into
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
|
hlenke
left a comment
There was a problem hiding this comment.
Needs changes · The claims hold up; before merging, re-run the failed preview-checks job and settle the Prometheus hunk against #776, which deletes this page and already says the same thing.
Merge readiness
- CI:
preview-checksfailed on 2026-07-24 in the step "Wait for Mintlify preview URL" after about 20 minutes, so the Checkly checks against the preview never ran. The job log has expired (HTTP 410), so I can only see the failing step. It looks like a preview timeout rather than a content problem: re-run the job or push a commit. All other checks pass. - Overlap with #776: #776 deletes
integrations/observability/prometheus-v2.mdx(redirect to/integrations/observability/prometheus), and its newprometheus.mdxalready states that histograms expose only the+Infbucket and that_sum / _countaverages should be used instead ofhistogram_quantile(). If #776 lands first, drop this PR's Prometheus hunk; it would be a modify/delete conflict and is covered there. If this PR lands first, #776 needs a rebase. #446 also editsprometheus-v2.mdx, in a different hunk. - Staleness: since the base,
mainchangedprometheus-v2.mdx(bc4370b, 4d01c2a, 749370d) andlearn/playwright/authentication.mdx(9f8c2ac repointed links). None of them touch these lines; GitHub reports the PR as mergeable. - Cross-file impact (
pr_impact.py): nothing reported. The new anchor#sms-and-email-one-time-passcodesmatches the Mintlify slug of the new heading, and/platform/variablesexists.
Inline comments: 1 Important, 2 Wording. The PR adds three em dashes in new prose; the suggestions remove them.
What I checked
- Read the whole diff from the merge base
ab2628fbto the head, and the three pages onorigin/main. gh pr checks 449,gh run view 30095771644(failed step: "Wait for Mintlify preview URL";--log-failedreturns HTTP 410),gh pr view 449 --json mergeable,mergeStateStatus,git log <base>..origin/mainon the touched files, open PRs touching the same files,scripts/pr_impact.py,scripts/mintslug.pyrule for the new heading.- Histogram claim: the Prometheus v2 exporter goldens in monorepo origin/main@28c905592a (
apps/backend/api/src/modules/public-api/prometheus-v2/__tests__/__goldens__/) contain onlyle="+Inf"bucket lines, in every golden file. Sohistogram_quantile()has no finite buckets to work with. #776's new page says the same. - Percentile alternative: the OpenAPI spec on
mainlistsresponseTime_p95andresponseTime_p99among themetricsvalues ofGET /v1/analytics/api-checks/{id}(and the sibling analytics endpoints). mainhas no other page covering SMS or email OTP in Playwright (git grepoverlearn/,detect/,guides/), so the new section fills a real gap. The third-party services it names (Mailosaur, Mailslurp, the Gmail API, Twilio) are offered as examples; I did not test them.- No end-to-end test: the PR adds no runnable code samples, and the histogram claim is settled by the exporter goldens.
| The histogram metrics exposed by the Checkly exporter only ship a single `le="+Inf"` bucket — they don't include the finite bucket boundaries that `histogram_quantile()` needs. That means percentile queries such as `histogram_quantile(0.95, rate(checkly_api_check_timing_seconds_bucket[5m]))` will not return a useful value. | ||
|
|
||
| Use the `_sum / _count` average pattern shown above for aggregations. If you need percentile-style latency signals, compute them in your alerting layer from the raw check run data rather than from the `_bucket` series. |
There was a problem hiding this comment.
Important · The histogram claim is right (the exporter goldens only contain le="+Inf" buckets). But "compute them in your alerting layer from the raw check run data" leaves the reader with nothing to do: the exporter does not expose raw run data. The Analytics API returns percentiles directly (responseTime_p95, responseTime_p99 are metrics values of GET /v1/analytics/api-checks/{id} in the OpenAPI spec). This version also drops the em dash.
| The histogram metrics exposed by the Checkly exporter only ship a single `le="+Inf"` bucket — they don't include the finite bucket boundaries that `histogram_quantile()` needs. That means percentile queries such as `histogram_quantile(0.95, rate(checkly_api_check_timing_seconds_bucket[5m]))` will not return a useful value. | |
| Use the `_sum / _count` average pattern shown above for aggregations. If you need percentile-style latency signals, compute them in your alerting layer from the raw check run data rather than from the `_bucket` series. | |
| The histogram metrics expose only the `le="+Inf"` bucket, without the finite bucket boundaries that `histogram_quantile()` needs. A percentile query such as `histogram_quantile(0.95, rate(checkly_api_check_timing_seconds_bucket[5m]))` does not return a useful value. | |
| Use the `_sum / _count` averages shown above. For percentiles, use the [Analytics API](/api-reference/analytics/api-checks), which returns metrics such as `responseTime_p95` and `responseTime_p99`. |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
hlenke
left a comment
There was a problem hiding this comment.
Ready to merge · All three comments from the last review are fixed, and the Prometheus hunk is gone now that #776 covers it on main. The one failing check, preview-checks, is not caused by this PR and is not required.
This review covers the commits since 56f58d83 (the SHA I reviewed last): 920892f, 1053dfc, the merge of main (a42388f), and 2634fc7.
What changed since the last review
learn/playwright/authentication.mdx: the intro paragraph of the new section now uses the suggested wording, with no em dash.learn/playwright/bypass-totp.mdx: the<Note>now uses the suggested wording, with no em dash and without the list that repeated the linked section.integrations/observability/prometheus-v2.mdx: the histogram<Note>is removed. That is the right call. #776 has merged (e905ff0) and deletedprometheus-v2.mdx. The newintegrations/observability/prometheus.mdxonmain(line 191) already says histograms expose only the+Infbucket and that readers should use_sum / _count, nothistogram_quantile(). With the hunk removed, the PR no longer touches that file, so there is no modify/delete conflict.
Merge readiness
- CI:
preview-checksfails in the step "Verify schema-generated API pages on preview" (run 41755484864). 23 status-page, status-page-incident, and status-page-service API pages return 404 on this PR's Mintlify preview. This PR does not cause it: its diff from the merge base touches only the twolearn/playwright/pages, notdocs.jsonorapi-reference/. The same 23 URLs also return 404 on other preview branches in the same hour, for example run 41757766874 (mintlify/22bf3c27). The page also loads on production (/docs/api-reference/status-pages/create-a-new-status-page/returns 200), and it passed on theplaywright-vs-cypress-2026preview at 02:04. I still get a 404 for it on this PR's preview now. GitHub reportsmergeStateStatus: UNSTABLE, so the check is not required. Re-running it after the preview rebuilds is optional. If it keeps failing on several branches, it is a preview problem to raise separately. - Conflicts and staleness: GitHub reports the PR as mergeable. Since the merge base
5ff4b9c7,mainhas not changed either touched file. - Overlap: no other open PR touches
learn/playwright/authentication.mdxorlearn/playwright/bypass-totp.mdx. - Cross-file impact (
pr_impact.py): nothing reported. - Optional: the PR title and description still mention the Prometheus histogram change. Updating them would keep the squash commit message accurate, for example "Fill gaps from assistant conversations: SMS/email OTP".
Inline comments: none.
What I checked
scripts/eligible_prs.sh: PR 449 was listed asreview, last reviewed at56f58d83ce5d3cec62e4bb2b90d3ebd405f6f28a.git diff 56f58d83..2634fc7fon the touched files, to see what changed since the last review. Most of theprometheus-v2.mdxlines in that range come from the merge ofmain, not from this PR.- The full diff from the merge base
5ff4b9c7to the head: 13 added lines inauthentication.mdxand 4 inbypass-totp.mdx. I compared it with the inline comments from the last review: all three suggestions were applied as written, and the Important comment on the histogram note was settled by removing the note. - The heading outline of
authentication.mdxat the head: the new### SMS and email one-time passcodessection sits under## Two-Factor Authentication / TOTP, after the TOTP example and before## Passkey & WebAuthn. That is the right place for it. git greponorigin/main:integrations/observability/prometheus.mdx:191covers the+Infbucket andhistogram_quantile(), andprometheus-v2.mdxno longer exists (redirected to/integrations/observability/prometheus).gh pr checks 449,gh run view 41755484864 --log-failed,gh run view 41757766874 --log-failed(same 404s on another branch), the log of the passing run 41758095078,.github/scripts/check-api-preview.mjsonmain, andcurlagainst this PR's preview, theplaywright-vs-cypress-2026preview, and production forapi-reference/status-pages/create-a-new-status-page/.gh pr view 449 --json mergeable,mergeStateStatus,git log 5ff4b9c7..origin/mainon the touched files,gh pr list --json number,filesfor overlap, andscripts/pr_impact.py.- No end-to-end test: the PR adds no runnable code samples, and nothing has changed since the last review that needs one.
Summary
Close two recurring documentation gaps surfaced by recent assistant conversations: how to test SMS/email one-time passcodes in Playwright, and why
histogram_quantile()doesn't return useful values against Checkly's Prometheus histogram metrics.Changes
learn/playwright/authentication.mdx: added an SMS and email one-time passcodes section under the 2FA/TOTP heading with the practical patterns (test-mode bypass, programmable inbox/SMS providers, reading from your own backend).learn/playwright/bypass-totp.mdx: added a callout up front clarifying the article is TOTP-only and pointing readers to the new SMS/email section.integrations/observability/prometheus-v2.mdx: added a note in the Histogram averages section explaining that the exporter only ships anle="+Inf"bucket, sohistogram_quantile()won't work, and reiterating the_sum / _countpattern.Context
Signal from the docs assistant: an unanswered thread on phone/email OTP where the assistant explicitly acknowledged the docs "do not cover phone SMS or email-based OTP flows", and a Prometheus thread where a user hit the missing bucket boundaries and the docs only covered averages.