mirror of
https://github.com/snapotter-hq/SnapOtter.git
synced 2026-08-03 07:46:42 +02:00
fix(telemetry): data-quality pass (opt-in noise, onboarding split, file_count, OIDC) (#614)
Five fixes to the PostHog event stream, from an audit of what we actually collect versus what's flowing in. Each one is test-first. ## What changed **Silenced the `$opt_in` noise.** `initAnalytics` called `opt_in_capturing()` on every page load to clear a stale opt-out flag, and posthog-js emits an `$opt_in` event on every call. That was 10k+ events a month (up to 55 per user) carrying no signal: analytics is on by default with an admin opt-out, so there is no per-user consent to record. Both call sites now pass `captureEventName: false`. **Split the onboarding survey out of `feedback_submitted`.** The onboarding usage survey rode the same event as real feedback, so about 93% of "feedback" was actually onboarding profiling. It now emits `onboarding_survey_submitted`, so feedback metrics mean feedback again. **Set `pipeline_executed.file_count`.** It was declared in the properties interface but never populated. A pure `pipelineExecutedProps` helper now derives it (batch size for a batch run, else 1) and is shared by the success and failure paths, which also drops a duplicated payload. **Tracked OIDC login failures.** All six OIDC callback failure branches bumped the Prometheus counter and wrote an audit log but never emitted `auth_login_failed`. A `recordOidcFailure` helper mirrors the password path. **Added `TELEMETRY.md`.** A contributor-facing event dictionary: every event, its properties, where it fires, and the privacy invariants, with the allowlists as source of truth. A drift test fails if any `ANALYTICS_EVENTS` value goes undocumented. I left the published telemetry guide (`apps/docs/guide/telemetry.md`) alone. It is high-level and still accurate, and editing it would pull in the 21-locale stale-gate for no gain. ## Verification - Unit (63 tests): `analytics-events`, `telemetry-doc-drift`, `api/analytics`, `web/analytics`, `worker.behavior` - Integration (41 tests): `oidc-auth`, `feedback` - Full typecheck across all 9 workspaces - Biome clean on the changed files All green locally.
This commit is contained in:
+42
-14
@@ -31,6 +31,7 @@ import {
|
||||
getBundleForTool,
|
||||
getOptionalBundleForTool,
|
||||
isToolInputError,
|
||||
type PipelineExecutedProperties,
|
||||
TOOLS,
|
||||
} from "@snapotter/shared";
|
||||
import { type Job, UnrecoverableError, Worker } from "bullmq";
|
||||
@@ -750,6 +751,33 @@ function contentTypeForFilename(name: string): string {
|
||||
return map[ext] ?? "application/octet-stream";
|
||||
}
|
||||
|
||||
/**
|
||||
* Build the pipeline_executed analytics payload. Shared by the success and
|
||||
* failure paths so is_batch and file_count are derived in one place. file_count
|
||||
* is the batch size for a batch-finalize job, otherwise 1 for a single-file
|
||||
* pipeline run.
|
||||
*/
|
||||
export function pipelineExecutedProps(
|
||||
data: Pick<ToolJobData, "kind" | "totalFiles">,
|
||||
totalSteps: number,
|
||||
toolIds: string[],
|
||||
durationMs: number,
|
||||
status: "completed" | "failed",
|
||||
) {
|
||||
// `satisfies` (not a return-type annotation) validates the shape against
|
||||
// PipelineExecutedProperties while keeping the inferred anonymous type, which
|
||||
// stays assignable to trackEvent's Record<string, unknown> param (a named
|
||||
// interface would not be).
|
||||
return {
|
||||
step_count: totalSteps,
|
||||
tool_ids: toolIds,
|
||||
is_batch: data.kind === "batch-finalize",
|
||||
file_count: data.totalFiles ?? 1,
|
||||
duration_ms: durationMs,
|
||||
status,
|
||||
} satisfies PipelineExecutedProperties;
|
||||
}
|
||||
|
||||
async function processPipelineFinalize(job: Job<ToolJobData>): Promise<ToolJobResult> {
|
||||
const data = job.data;
|
||||
const startTime = Date.now();
|
||||
@@ -818,13 +846,13 @@ async function processPipelineFinalize(job: Job<ToolJobData>): Promise<ToolJobRe
|
||||
if (analyticsEnabled()) {
|
||||
void trackEvent(
|
||||
ANALYTICS_EVENTS.PIPELINE_EXECUTED,
|
||||
{
|
||||
step_count: totalSteps,
|
||||
tool_ids: steps.map((s) => s.toolId),
|
||||
is_batch: data.kind === "batch-finalize",
|
||||
duration_ms: Date.now() - startTime,
|
||||
status: "failed",
|
||||
},
|
||||
pipelineExecutedProps(
|
||||
data,
|
||||
totalSteps,
|
||||
steps.map((s) => s.toolId),
|
||||
Date.now() - startTime,
|
||||
"failed",
|
||||
),
|
||||
data.analyticsDistinctId,
|
||||
);
|
||||
}
|
||||
@@ -905,13 +933,13 @@ async function processPipelineFinalize(job: Job<ToolJobData>): Promise<ToolJobRe
|
||||
if (analyticsEnabled()) {
|
||||
void trackEvent(
|
||||
ANALYTICS_EVENTS.PIPELINE_EXECUTED,
|
||||
{
|
||||
step_count: totalSteps,
|
||||
tool_ids: steps.map((s) => s.toolId),
|
||||
is_batch: data.kind === "batch-finalize",
|
||||
duration_ms: Date.now() - startTime,
|
||||
status: "completed",
|
||||
},
|
||||
pipelineExecutedProps(
|
||||
data,
|
||||
totalSteps,
|
||||
steps.map((s) => s.toolId),
|
||||
Date.now() - startTime,
|
||||
"completed",
|
||||
),
|
||||
data.analyticsDistinctId,
|
||||
);
|
||||
}
|
||||
|
||||
@@ -153,9 +153,15 @@ export async function captureFeedback(
|
||||
): Promise<void> {
|
||||
try {
|
||||
if (!analyticsEnabled() || !posthogClient) return;
|
||||
// The onboarding usage survey is a profiling questionnaire, not feedback, so
|
||||
// it gets its own event name and feedback_submitted stays genuine feedback.
|
||||
const event =
|
||||
properties.source === "onboarding"
|
||||
? ANALYTICS_EVENTS.ONBOARDING_SURVEY_SUBMITTED
|
||||
: ANALYTICS_EVENTS.FEEDBACK_SUBMITTED;
|
||||
posthogClient.capture({
|
||||
distinctId: distinctId ?? (await getInstanceId()),
|
||||
event: ANALYTICS_EVENTS.FEEDBACK_SUBMITTED,
|
||||
event,
|
||||
properties: cleanFeedbackProperties(properties),
|
||||
});
|
||||
} catch {
|
||||
|
||||
@@ -102,6 +102,16 @@ function redirectToLogin(reply: FastifyReply, errorCode: string): void {
|
||||
reply.redirect(`/login?error=${errorCode}`);
|
||||
}
|
||||
|
||||
/**
|
||||
* Record a failed OIDC login: bump the Prometheus auth counter and emit the
|
||||
* auth_login_failed analytics event, mirroring the password login path so
|
||||
* OIDC failures are visible in analytics too.
|
||||
*/
|
||||
function recordOidcFailure(): void {
|
||||
authAttempts.inc({ method: "oidc", result: "failure" });
|
||||
void trackEvent(ANALYTICS_EVENTS.AUTH_LOGIN_FAILED, { method: "oidc" });
|
||||
}
|
||||
|
||||
// ── OIDC Routes ───────────────────────────────────────────────────
|
||||
|
||||
export async function oidcRoutes(app: FastifyInstance): Promise<void> {
|
||||
@@ -202,7 +212,7 @@ export async function oidcRoutes(app: FastifyInstance): Promise<void> {
|
||||
{ error: query.error, description: query.error_description },
|
||||
"OIDC IdP returned error",
|
||||
);
|
||||
authAttempts.inc({ method: "oidc", result: "failure" });
|
||||
recordOidcFailure();
|
||||
await audit("OIDC_LOGIN_FAILED", {
|
||||
reason: sanitizeAuditInput(String(query.error)),
|
||||
});
|
||||
@@ -233,7 +243,7 @@ export async function oidcRoutes(app: FastifyInstance): Promise<void> {
|
||||
});
|
||||
} catch (err) {
|
||||
request.log.error({ err }, "OIDC token exchange failed");
|
||||
authAttempts.inc({ method: "oidc", result: "failure" });
|
||||
recordOidcFailure();
|
||||
await audit("OIDC_LOGIN_FAILED", { reason: "token_exchange_failed" });
|
||||
return redirectToLogin(reply, "oidc_auth_failed");
|
||||
}
|
||||
@@ -242,7 +252,7 @@ export async function oidcRoutes(app: FastifyInstance): Promise<void> {
|
||||
const claims = tokenResponse.claims();
|
||||
if (!claims) {
|
||||
request.log.error("OIDC callback: no ID token claims");
|
||||
authAttempts.inc({ method: "oidc", result: "failure" });
|
||||
recordOidcFailure();
|
||||
await audit("OIDC_LOGIN_FAILED", { reason: "no_id_token" });
|
||||
return redirectToLogin(reply, "oidc_auth_failed");
|
||||
}
|
||||
@@ -270,7 +280,7 @@ export async function oidcRoutes(app: FastifyInstance): Promise<void> {
|
||||
});
|
||||
|
||||
if (result.action === "denied" || !result.user) {
|
||||
authAttempts.inc({ method: "oidc", result: "failure" });
|
||||
recordOidcFailure();
|
||||
if (result.deniedReason === "user_limit_reached") {
|
||||
return redirectToLogin(reply, "oidc_user_limit_reached");
|
||||
}
|
||||
@@ -294,7 +304,7 @@ export async function oidcRoutes(app: FastifyInstance): Promise<void> {
|
||||
{ err, userId: resolvedUser.id },
|
||||
"OIDC callback: failed to read MFA enrollment status",
|
||||
);
|
||||
authAttempts.inc({ method: "oidc", result: "failure" });
|
||||
recordOidcFailure();
|
||||
await audit("OIDC_LOGIN_FAILED", {
|
||||
userId: resolvedUser.id,
|
||||
username: resolvedUser.username,
|
||||
@@ -328,7 +338,7 @@ export async function oidcRoutes(app: FastifyInstance): Promise<void> {
|
||||
}
|
||||
|
||||
if (mfaOutcome === "enrollment_required") {
|
||||
authAttempts.inc({ method: "oidc", result: "failure" });
|
||||
recordOidcFailure();
|
||||
await audit("OIDC_LOGIN_FAILED", {
|
||||
userId: resolvedUser.id,
|
||||
username: resolvedUser.username,
|
||||
|
||||
Reference in New Issue
Block a user