diff --git a/apps/server/src/libs/cli-telemetry.ts b/apps/server/src/libs/cli-telemetry.ts index bdb1bf90e..4fd277f0b 100644 --- a/apps/server/src/libs/cli-telemetry.ts +++ b/apps/server/src/libs/cli-telemetry.ts @@ -1,5 +1,5 @@ import { getLogger } from "@logtape/logtape"; -import { type EventProps, Events } from "@openstatus/analytics"; +import { type Analytics, type EventProps, Events } from "@openstatus/analytics"; import { cacheKeys } from "@/libs/cache-keys"; import { redis } from "@/libs/clients"; @@ -83,10 +83,6 @@ export async function claimCliCommandEvent( }; } -type Analytics = { - track: (event: EventProps & Record) => Promise; -}; - /** * Sends this run's `cli_command` through `analytics` if this request is the * first of the run to claim it. Claim only once analytics is set up, and give @@ -105,7 +101,12 @@ export async function trackCliCommand( } catch (error) { await redis .del(cacheKeys.cliInvocation(workspaceId, cli.invocation)) - .catch(() => {}); + .catch(() => { + logger.warn( + "Failed to release CLI invocation claim for workspace {workspaceId}", + { workspaceId }, + ); + }); throw error; } } diff --git a/apps/server/src/libs/middlewares/track.test.ts b/apps/server/src/libs/middlewares/track.test.ts index 566eb795e..bf21cb126 100644 --- a/apps/server/src/libs/middlewares/track.test.ts +++ b/apps/server/src/libs/middlewares/track.test.ts @@ -84,6 +84,16 @@ describe("apiTrackMiddleware", () => { }); }); + test("skips requests that match no route", async () => { + const res = await makeApp().request("/nope", { + headers: { "user-agent": "curl/8" }, + }); + await flush(); + + expect(res.status).toBe(404); + expect(mockSetupAnalytics).not.toHaveBeenCalled(); + }); + test("skips requests without a workspace", async () => { const res = await makeApp({ withWorkspace: false }).request("/whoami", { headers: cliHeaders(crypto.randomUUID()), diff --git a/apps/server/src/libs/middlewares/track.ts b/apps/server/src/libs/middlewares/track.ts index 915b2cf06..628f48d43 100644 --- a/apps/server/src/libs/middlewares/track.ts +++ b/apps/server/src/libs/middlewares/track.ts @@ -6,7 +6,7 @@ import { setupAnalytics, } from "@openstatus/analytics"; import type { Context, Next } from "hono"; -import { routePath } from "hono/route"; +import { matchedRoutes } from "hono/route"; import { apiAnalyticsIdentity } from "@/libs/analytics-identity"; import { parseCliHeaders, trackCliCommand } from "@/libs/cli-telemetry"; @@ -56,7 +56,7 @@ export function trackMiddleware(event: EventProps, eventProps?: string[]) { * the first request of each CLI run fires one `cli_command`. * * Mount after `authMiddleware`; requests it rejects carry no workspace and are - * skipped. Per-route domain events stay with `trackMiddleware`. + * skipped, as are requests that match no route. Per-route domain events stay with `trackMiddleware`. */ export function apiTrackMiddleware() { return async (c: Context<{ Variables: Variables }, "/*">, next: Next) => { @@ -65,6 +65,12 @@ export function apiTrackMiddleware() { const workspace = c.get("workspace"); if (!workspace) return; + // `use()` middlewares register as `ALL`; with no handler route matched + // (a 404 on an unknown path) the only pattern left is `/*`, which says + // nothing about the endpoint, so those aren't counted. + const route = matchedRoutes(c).findLast((r) => r.method !== "ALL"); + if (!route) return; + const cli = parseCliHeaders(c.req.raw.headers); const success = c.res.status.toString().startsWith("2") && !c.error; @@ -74,7 +80,7 @@ export function apiTrackMiddleware() { analytics.track({ ...Events.ApiRequest, service: "v1", - method: `${c.req.method} ${routePath(c, -1)}`, + method: `${c.req.method} ${route.path}`, success, ...(cli ? { cliCommand: cli.command } : {}), ...(cli?.version ? { cliVersion: cli.version } : {}), diff --git a/apps/server/src/libs/test/doubles/analytics.mock.ts b/apps/server/src/libs/test/doubles/analytics.mock.ts index dfe713439..3ac496513 100644 --- a/apps/server/src/libs/test/doubles/analytics.mock.ts +++ b/apps/server/src/libs/test/doubles/analytics.mock.ts @@ -2,8 +2,10 @@ // real Events/parseInputToProps but replaces setupAnalytics with a spy (exposed // on globalThis.__analyticsSpies) so tests assert tracking without OpenPanel. export { + type Analytics, type EventProps, Events, + type IdentifyProps, parseInputToProps, } from "@openstatus/analytics-real"; diff --git a/packages/analytics/src/server.ts b/packages/analytics/src/server.ts index 2fe43e3f5..fcff971ac 100644 --- a/packages/analytics/src/server.ts +++ b/packages/analytics/src/server.ts @@ -99,17 +99,19 @@ export async function setupAnalytics(props: IdentifyProps) { } const groupId = props.workspaceId ? `ws_${props.workspaceId}` : undefined; + // profileId and groups go on each event rather than relying on the client + // state identify()/setGroup() leave behind, which the cache below skips. const track = (opts: EventProps & TrackProperties) => { const { name, ...rest } = opts; - return op.track(name, groupId ? { ...rest, groups: [groupId] } : rest); + return op.track(name, { + ...(props.userId ? { profileId: props.userId } : {}), + ...rest, + ...(groupId ? { groups: [groupId] } : {}), + }); }; const key = identityKey(props); - if (wasIdentitySent(key)) { - // A profileId-only identify sets the id `track()` reads without a request. - if (props.userId) op.identify({ profileId: props.userId }); - return { track }; - } + if (wasIdentitySent(key)) return { track }; if (props.userId) { const [firstName, lastName] = props.fullName?.split(" ") || []; @@ -138,6 +140,8 @@ export async function setupAnalytics(props: IdentifyProps) { return { track }; } +export type Analytics = Awaited>; + /** * Noop analytics for development environment */