feat: instrument registration/event routes + metrics by HJyup · Pull Request #73 · compsoc-edinburgh/events.comp-soc.com

HJyup · 2026-08-22T14:37:51Z

No description provided.

vercel · 2026-08-22T14:37:57Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
events-comp-soc-com-web	Ready	Preview	Aug 23, 2026 3:59pm

Copilot

Pull request overview

This PR introduces OpenTelemetry-based observability for the API service by adding an OTel SDK bootstrap that runs before the Fastify app starts, alongside dependency and runtime entrypoint updates.

Changes:

Add OpenTelemetry NodeSDK setup (OTLP trace exporter + auto-instrumentations + Fastify instrumentation) and load it via a new bootstrap entrypoint.
Update API runtime entrypoints (npm start, npm dev, Docker CMD) to use the new bootstrap file.
Refactor the Clerk webhook route definition (no functional change intended).

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
pnpm-lock.yaml	Locks new OpenTelemetry/Fastify OTel dependencies and transitive packages.
apps/api/src/telemetry/sdk.ts	Initializes the OpenTelemetry NodeSDK and instrumentations.
apps/api/src/telemetry/config.ts	Defines telemetry service/exporter configuration constants.
apps/api/src/modules/webhooks/clerk.ts	Restructures Clerk webhook route handler.
apps/api/src/bootstrap.ts	New entrypoint to start telemetry before importing the app.
apps/api/package.json	Switch start/dev scripts to use the bootstrap entrypoint and add OTel deps.
apps/api/Dockerfile	Updates container startup command to run the bootstrap entrypoint.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

apps/api/src/telemetry/config.ts:3

Telemetry configuration is hardcoded in source control (service name/version and OTLP endpoint). This makes deployments difficult and can accidentally point production at localhost. Prefer reading from environment variables with sensible defaults.

export const OTEL_SERVICE_NAME = "events.compsoc.api";
export const OTEL_SERVICE_VERSION = "1.0.0";
export const OTEL_BASE_URL = "http://127.0.0.1:4318";

apps/api/src/telemetry/sdk.ts:13

deployment.environment.name is currently hardcoded to "development", so traces from other environments will be mislabeled. Prefer deriving this from process.env.NODE_ENV (or a dedicated OTEL env var) with a default.

  resource: resourceFromAttributes({
    "service.name": OTEL_SERVICE_NAME,
    "service.version": OTEL_SERVICE_VERSION,
    "deployment.environment.name": "development",
  }),

apps/api/src/telemetry/sdk.ts:37

The telemetry SDK is started as a fire-and-forget side effect. If startup fails, the error is currently unhandled, and bootstrap continues. Consider awaiting startup and surfacing failures so the process doesn’t run without telemetry unintentionally.


sdk.start();

Copilot

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

Suppressed comments (6)

Previously missed (1) — in code that hasn't changed since the last review.

apps/api/src/telemetry/config.ts:3

Telemetry config is hard-coded (service version, exporter URL). This will likely be wrong in non-dev environments and makes container deployments harder. Prefer reading these from environment variables with sane defaults.

export const OTEL_SERVICE_NAME = "events.compsoc.api";
export const OTEL_SERVICE_VERSION = "1.0.0";
export const OTEL_BASE_URL = "http://127.0.0.1:4318";

apps/api/src/modules/webhooks/clerk.ts:29

Spelling in the comment: "recieved" -> "received".

// User data, which recieved from clerk on registration

apps/api/src/telemetry/sdk.ts:13

deployment.environment.name is hard-coded to "development", so traces from staging/prod will be mislabeled unless this file is edited per environment. Derive it from env (e.g. OTEL_ENVIRONMENT/NODE_ENV) with a default.

  resource: resourceFromAttributes({
    "service.name": OTEL_SERVICE_NAME,
    "service.version": OTEL_SERVICE_VERSION,
    "deployment.environment.name": "development",
  }),

apps/api/src/telemetry/sdk.ts:26

The HTTP instrumentation only ignores requests where request.url === "/health". If the health endpoint is called with a query string (e.g. /health?full=1) it will still be traced. Consider a prefix match instead.

      "@opentelemetry/instrumentation-http": {
        ignoreIncomingRequestHook: (request) => request.url === "/health",
      },

apps/api/src/telemetry/sdk.ts:37

NodeSDK.start() is async; calling it without awaiting/catching means startup failures can become unhandled rejections, and the app may import/load modules before instrumentation is fully initialized (missing early spans). It also never shuts down, so spans may be dropped on SIGTERM/SIGINT. Consider awaiting start (top-level await is OK in ESM) and registering shutdown handlers.

sdk.start();

apps/api/src/telemetry/sdk.ts:18

The PR title mentions "metrics", but this SDK setup only configures a trace exporter (no metricReader/metrics exporter is set up). If metrics are intended, you'll need to add a metrics exporter/reader; otherwise consider updating the PR title to avoid confusion.

const sdk = new NodeSDK({
  resource: resourceFromAttributes({
    "service.name": OTEL_SERVICE_NAME,
    "service.version": OTEL_SERVICE_VERSION,
    "deployment.environment.name": "development",
  }),

  traceExporter: new OTLPTraceExporter({
    url: `${OTEL_BASE_URL}/v1/traces`,
  }),

Copilot

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (5)

Previously missed (1) — in code that hasn't changed since the last review.

apps/api/src/telemetry/config.ts:3

Telemetry config is hard-coded (service name/version and collector base URL). This will likely be wrong in production deployments and makes it hard to configure via environment variables.

export const OTEL_SERVICE_NAME = "events.compsoc.api";
export const OTEL_SERVICE_VERSION = "1.0.0";
export const OTEL_BASE_URL = "http://127.0.0.1:4318";

apps/api/src/modules/webhooks/clerk.ts:29

Typo in comment: “recieved” should be “received”.

// User data, which recieved from clerk on registration

apps/api/src/modules/webhooks/clerk.ts:92

request.rawBody is asserted non-null when verifying the Svix signature. If the request doesn’t hit the custom JSON parser (e.g. different/missing Content-Type), this will throw and be reported as an “Invalid webhook signature”, which is misleading.

    const wh = new Webhook(webhookSecret);
    let event: ClerkWebhookEvent;

    try {
      event = wh.verify(request.rawBody!, {
        "svix-id": svixId,

apps/api/src/telemetry/sdk.ts:37

sdk.start() is async but is not awaited. That can race app startup (instrumentations may not be fully registered before the server loads) and any startup failure becomes an unhandled rejection.

sdk.start();

apps/api/src/telemetry/sdk.ts:13

deployment.environment.name is hard-coded to "development". This will mis-label spans in non-dev environments; it should come from configuration (e.g. NODE_ENV / OTEL_DEPLOYMENT_ENVIRONMENT).

import { OTEL_SERVICE_NAME, OTEL_SERVICE_VERSION, OTEL_BASE_URL } from "./config.js";

const sdk = new NodeSDK({
  resource: resourceFromAttributes({
    "service.name": OTEL_SERVICE_NAME,
    "service.version": OTEL_SERVICE_VERSION,
    "deployment.environment.name": "development",
  }),

Copilot

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

apps/api/src/telemetry/config.ts:3

These telemetry constants are hard-coded to development defaults (service version and OTLP endpoint). This will silently point production deployments at localhost unless overridden in code; prefer reading from environment variables with sensible fallbacks.

export const OTEL_SERVICE_NAME = "events.compsoc.api";
export const OTEL_SERVICE_VERSION = "1.0.0";
export const OTEL_BASE_URL = "http://127.0.0.1:4318";

apps/api/src/telemetry/sdk.ts:37

NodeSDK.start() should be awaited and paired with a shutdown handler so traces flush on SIGTERM/SIGINT (especially in Docker). As written, startup errors can be missed and spans may be dropped on process exit.

sdk.start();

apps/api/src/modules/webhooks/clerk.ts:29

Typo in comment: "recieved" → "received".

// User data, which recieved from clerk on registration

@@ -0,0 +1,47 @@
+import { Attributes, Span, SpanStatusCode, trace } from "@opentelemetry/api";


Copilot

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

apps/api/src/modules/registration/service.ts:182

The compsoc.registration.event_capacity span attribute is sometimes a number (elsewhere) and sometimes the string "unlimited". Keeping a consistent attribute type avoids downstream query/aggregation issues in OTel backends.

          span.setAttributes({
            "compsoc.registration.active_count": activeCount,
            "compsoc.registration.event_capacity": event.capacity ?? "unlimited",
          });

apps/api/src/modules/registration/service.ts:247

This route returns updatedCount as the requested count (data.userIds.length) even when fewer (or zero) rows were actually updated. Since updateStatusBatch() returns the updated rows, the response should reflect updated.length for accuracy.

          span.setAttribute("compsoc.registration.updated_count", updated.length);
          setOutcome(updated.length === 0 ? "no_matches" : "updated");

          return { updatedCount: data.userIds.length };

apps/api/src/modules/webhooks/clerk.ts:30

Spelling: "recieved" -> "received".

// User data, which recieved from clerk on registration

Copilot

Pull request overview

Copilot reviewed 17 out of 18 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

apps/api/src/modules/registration/service.ts:247

batchUpdateStatus computes the number of updated rows (updated.length) but returns updatedCount as the requested count (data.userIds.length). This can mislead API consumers and log/trace data when some userIds don't match registrations.

          span.setAttribute("compsoc.registration.updated_count", updated.length);
          setOutcome(updated.length === 0 ? "no_matches" : "updated");

          return { updatedCount: data.userIds.length };

apps/api/src/modules/webhooks/clerk.ts:30

Typo in comment: "recieved" should be "received".

// User data, which recieved from clerk on registration

Copilot

Pull request overview

Copilot reviewed 17 out of 18 changed files in this pull request and generated no new comments.

Suppressed comments (3)

apps/api/src/modules/webhooks/clerk.ts:30

The comment has a spelling mistake: "recieved" → "received".
apps/api/src/bootstrap.ts:7
NodeSDK.start() is async in the OpenTelemetry Node SDK; calling it without await can lead to instrumentation not being fully initialised before the app starts, and startup failures may become unhandled rejections.

const { sdk } = await import("./telemetry/sdk.js");
sdk.start();

await import("./app.js");

apps/api/src/modules/registration/service.ts:247

batchUpdateStatus returns updatedCount as the requested number of userIds, but registrationStore.updateStatusBatch() returns the actual updated rows. This can report incorrect counts when some userIds don't match registrations.

          return { updatedCount: data.userIds.length };

Copilot

Pull request overview

Copilot reviewed 17 out of 18 changed files in this pull request and generated 1 comment.

Suppressed comments (5)

apps/api/src/modules/webhooks/clerk.ts:31

Spelling in comment: "recieved" → "received".

// User data, which recieved from clerk on registration

apps/api/src/bootstrap.ts:7

NodeSDK.start() is async; calling it without awaiting means the server can start handling requests before instrumentation/exporters are ready, and startup errors may be silently dropped. Await the SDK start before importing/starting the app.

const { sdk } = await import("./telemetry/sdk.js");
sdk.start();

await import("./app.js");

apps/api/src/modules/registration/service.ts:178

spotsLeft uses a truthy check on event.capacity; if capacity can be 0, this incorrectly treats it as unlimited. Use an explicit null check (matching the other capacity logic in this file).

          const spotsLeft = event.capacity ? event.capacity - activeCount : Infinity;

apps/api/src/modules/registration/service.ts:248

batchUpdateStatus now computes updated.length and uses it for telemetry/outcome, but the API response still reports updatedCount as the requested count, which can be inaccurate when some userIds don’t match any registration. Return the actual updated count.

          span.setAttribute("compsoc.registration.updated_count", updated.length);
          setOutcome(updated.length === 0 ? "no_matches" : "updated");

          return { updatedCount: data.userIds.length };
        });

apps/api/drizzle.config.ts:11

@ts-expect-error will turn into a hard compile error if process becomes typed (unused expect-error). If the goal is just to silence missing Node globals in drizzle-kit’s config compilation, @ts-ignore is safer, or import node:process explicitly.

  dbCredentials: {
    // @ts-expect-error, it doesn't see types for the process
    url: process.env.DATABASE_URL!,
  },

+import { SpanStatusCode, trace, type Span } from "@opentelemetry/api";
+import { env } from "../../env.js";
+import z from "zod";


feat: add basic opentelemetry instrumentation for fastify

4345b37

HJyup self-assigned this Aug 22, 2026

Copilot AI lite review requested due to automatic review settings August 22, 2026 14:37

HJyup added the api API application related issues label Aug 22, 2026

Copilot started reviewing on behalf of HJyup August 22, 2026 14:38 View session

Copilot AI reviewed Aug 22, 2026

View reviewed changes

Comment thread apps/api/src/telemetry/sdk.ts Outdated

Comment thread apps/api/src/telemetry/config.ts Outdated

Comment thread apps/api/src/telemetry/sdk.ts Outdated

Comment thread apps/api/src/telemetry/sdk.ts

Comment thread apps/api/src/telemetry/sdk.ts

feat: add instrumentations on webhooks

37d6bd3

Copilot AI review requested due to automatic review settings August 22, 2026 15:20

vercel Bot deployed to Preview August 22, 2026 15:20 View deployment

Copilot started reviewing on behalf of HJyup August 22, 2026 15:20 View session

fix: remove duplications

a267bc1

vercel Bot deployed to Preview August 22, 2026 15:21 View deployment

Copilot AI reviewed Aug 22, 2026

View reviewed changes

Comment thread apps/api/src/modules/webhooks/clerk.ts

Copilot AI review requested due to automatic review settings August 22, 2026 15:24

Copilot started reviewing on behalf of HJyup August 22, 2026 15:24 View session

Copilot AI reviewed Aug 22, 2026

View reviewed changes

Comment thread apps/api/src/modules/webhooks/clerk.ts

HJyup added 2 commits August 22, 2026 16:31

fix: analytics data

08daec7

fix: fix tests

29a3e95

Copilot AI review requested due to automatic review settings August 22, 2026 15:31

vercel Bot deployed to Preview August 22, 2026 15:32 View deployment

Copilot started reviewing on behalf of HJyup August 22, 2026 15:32 View session

feat: add logs for registration

0b9fe3f

Copilot AI reviewed Aug 22, 2026

View reviewed changes

feat: add spans for registrations

16cdae4

Copilot AI review requested due to automatic review settings August 22, 2026 15:56

vercel Bot deployed to Preview August 22, 2026 15:57 View deployment

Copilot started reviewing on behalf of HJyup August 22, 2026 15:57 View session

Copilot AI reviewed Aug 22, 2026

View reviewed changes

Comment thread apps/api/src/telemetry/spans/registration-span.ts

@@ -0,0 +1,47 @@

import { Attributes, Span, SpanStatusCode, trace } from "@opentelemetry/api";

Comment thread apps/api/src/modules/registration/service.ts

fix: add env parsing

0622c62

Copilot AI review requested due to automatic review settings August 23, 2026 15:11

fix: update .env.test

0589196

Copilot started reviewing on behalf of HJyup August 23, 2026 15:11 View session

vercel Bot deployed to Preview August 23, 2026 15:12 View deployment

Copilot AI reviewed Aug 23, 2026

View reviewed changes

Comment thread apps/api/src/bootstrap.ts

Comment thread apps/api/drizzle.config.ts

fix: update env keys

ecf644f

Copilot AI review requested due to automatic review settings August 23, 2026 15:51

vercel Bot deployed to Preview August 23, 2026 15:52 View deployment

Copilot started reviewing on behalf of HJyup August 23, 2026 15:52 View session

Copilot AI reviewed Aug 23, 2026

View reviewed changes

Comment thread apps/api/src/modules/registration/service.ts

fix: rawBody acceptance check

a426ffe

Copilot AI review requested due to automatic review settings August 23, 2026 15:56

vercel Bot deployed to Preview August 23, 2026 15:56 View deployment

Copilot started reviewing on behalf of HJyup August 23, 2026 15:56 View session

fix: update parsing for websocket connection

e1f9e8c

vercel Bot deployed to Preview August 23, 2026 15:59 View deployment

HJyup merged commit 0a0994e into main Aug 23, 2026
5 checks passed

HJyup deleted the observability branch August 23, 2026 16:00

Copilot AI reviewed Aug 23, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 23, 2026 16:00

Copilot started reviewing on behalf of HJyup August 23, 2026 16:00 View session

Copilot AI reviewed Aug 23, 2026

View reviewed changes

Comment thread apps/api/src/modules/webhooks/clerk.ts

Comment on lines +7 to +9

import { SpanStatusCode, trace, type Span } from "@opentelemetry/api";

import { env } from "../../env.js";

import z from "zod";

		@@ -0,0 +1,47 @@
		import { Attributes, Span, SpanStatusCode, trace } from "@opentelemetry/api";

Uh oh!

Conversation

HJyup commented Aug 22, 2026

Uh oh!

vercel Bot commented Aug 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vercel Bot commented Aug 22, 2026 •

edited

Loading