Conversation
WalkthroughThis change instruments distributed tracing and error-tracing across the codebase: adds TraceContext, a tracing factory, and TracedError; creates and attaches per-request trace contexts in server request handling; propagates traceContext through APIs, TRPC, booking flows, payments, webhooks, workflows, and scheduling; replaces many logger usages with tracingLogger and attaches traceId to errors/responses (X-Trace-Id header); updates numerous function/class signatures and tests; adds a UI locale key and renders trace IDs on booking errors. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
keithwillcode
added
core
dosubot
bot
added
platform
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (20)
packages/features/webhooks/lib/handleWebhookScheduledTriggers.ts (1)
56-59: Fix crash on non‑JSON payloads; parse once and reuse.
JSON.parse(job.payload)will throw when payload is form-encoded (the header logic already anticipates non‑JSON). Parse once using the existingjsonParse, reuse the result for both Content‑Type and tracing, and avoid double parsing.Apply this diff:
@@ - const headers: Record<string, string> = { - "Content-Type": - !job.payload || jsonParse(job.payload) ? "application/json" : "application/x-www-form-urlencoded", - }; + const parsedJobPayload = jsonParse(job.payload) as { + id?: number; // booking id + endTime?: string; + triggerEvent?: string; + _traceContext?: TraceContext; + } | null; + + const headers: Record<string, string> = { + "Content-Type": + !job.payload || parsedJobPayload ? "application/json" : "application/x-www-form-urlencoded", + }; @@ - const parsedJobPayload = JSON.parse(job.payload) as { - id: number; // booking id - endTime: string; - triggerEvent: string; - _traceContext?: TraceContext; - }; + // parsedJobPayload is set above via jsonParse; may be null for non‑JSON payloads.Also applies to: 74-79
packages/features/bookings/lib/handleSeats/reschedule/owner/combineTwoSeatedBookings.ts (1)
106-114: Prisma: replace include with select (project only needed fields).Our guideline is to avoid include and explicitly select fields to reduce payload and risk.
Apply this diff:
- const updatedNewBooking = await prisma.booking.findUnique({ - where: { - id: newTimeSlotBooking.id, - }, - include: { - attendees: true, - references: true, - }, - }); + const updatedNewBooking = await prisma.booking.findUnique({ + where: { id: newTimeSlotBooking.id }, + select: { + id: true, + attendees: { + select: { + id: true, + email: true, + name: true, + timeZone: true, + locale: true, + phoneNumber: true, + bookingSeat: { select: { id: true } }, + }, + }, + // If addVideoCallDataToEvent needs specific fields, narrow these accordingly + references: true, + }, + });packages/features/bookings/lib/handleWebhookTrigger.ts (1)
54-55: Include traceContext in all handleWebhookTrigger invocations
- packages/features/bookings/lib/handleNewBooking.ts (lines 2161, 2301, 2313): add
traceContextto each call- packages/features/bookings/lib/handleSeats/handleSeats.ts (line 188): add
traceContext- ensure no remaining calls to
handleWebhookTrigger({…})omittraceContext—otherwise this will break at compile timepackages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.ts (1)
24-38: Pass traceContext to all scheduleNoShowTriggers invocations
Update each call to include the new required traceContext parameter to match its signature and avoid runtime errors:
- packages/trpc/server/routers/loggedInViewer/connectAndJoin.handler.ts (line 239)
- packages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.integration-test.ts (lines 112, 144)
- packages/features/bookings/lib/handleConfirmation.ts (line 463)
- packages/features/bookings/lib/handleNewBooking.ts (line 2402)
packages/lib/server/getServerErrorFromUnknown.ts (1)
128-131: Return type mismatch for string causesFunction promises
HttpErrorbut returnsError. Fix to returnHttpErrorand propagate trace info.- // @ts-expect-error https://github.com/tc39/proposal-error-cause - return new Error(cause, { cause }); + return new HttpError({ + statusCode: 500, + message: String(cause), + data: traceId ? { ...(tracedData ?? {}), traceId } : undefined, + });packages/features/ee/workflows/lib/reminders/scheduleMandatoryReminder.ts (1)
5-14: Remove unused logger import/variable
logger/logare no longer used; will trigger unused variable lint.-import logger from "@calcom/lib/logger"; import { withReporting } from "@calcom/lib/sentryWrapper"; @@ -const log = logger.getSubLogger({ prefix: ["[scheduleMandatoryReminder]"] });packages/features/bookings/lib/handleSeats/reschedule/owner/moveSeatedBookingToNewTimeSlot.ts (1)
43-58: Prisma: use select (no include), per repo guidelinesSwitch to
selectwith the same fields. Also includeid.- const newBooking: (Booking & { appsStatus?: AppsStatus[] }) | null = await prisma.booking.update({ + const newBooking: (Booking & { appsStatus?: AppsStatus[] }) | null = await prisma.booking.update({ where: { id: seatedBooking.id, }, data: { startTime: evt.startTime, endTime: evt.endTime, cancellationReason: rescheduleReason, }, - include: { - user: true, - references: true, - payment: true, - attendees: true, - }, + select: { + id: true, + user: true, + references: true, + payment: true, + attendees: true, + }, });packages/features/bookings/lib/handleSeats/reschedule/rescheduleSeatedBooking.ts (2)
35-53: Use Prisma select instead of includeProject policy: “only select data you need; never use include.” Replace
includewithselect.const newTimeSlotBooking = await prisma.booking.findFirst({ where: { startTime: dayjs(evt.startTime).toDate(), eventTypeId: eventType.id, status: BookingStatus.ACCEPTED, }, select: { id: true, uid: true, iCalUID: true, userId: true, - attendees: { - include: { - bookingSeat: true, - }, - }, + attendees: { + select: { + id: true, + name: true, + email: true, + timeZone: true, + locale: true, + bookingSeat: { + select: { + id: true, + referenceUid: true, + attendeeId: true, + }, + }, + }, + }, references: true, }, });
55-57: Propagate traceContext to EventManagerEventManager accepts an optional
traceContext. Pass it so event creation/reschedule logs/spans are linked to the current trace.- const eventManager = new EventManager({ ...organizerUser, credentials }); + const eventManager = new EventManager({ ...organizerUser, credentials }, undefined, traceContext);packages/features/bookings/lib/handleSeats/handleSeats.ts (1)
88-99: Use Prisma select instead of includePer project guideline, switch to
select.select: { uid: true, id: true, - attendees: { include: { bookingSeat: true } }, + attendees: { + select: { + id: true, + email: true, + name: true, + timeZone: true, + bookingSeat: { + select: { + id: true, + referenceUid: true, + }, + }, + }, + }, userId: true, references: true, startTime: true, user: true, status: true, smsReminderNumber: true, endTime: true, },packages/features/bookings/lib/handleNewBooking.ts (1)
520-521: Fix undefined variable: loggerWithEventDetails
loggerWithEventDetailsis referenced but never defined, causing a compile-time error. Use the tracing logger instead.- const emailsAndSmsHandler = new BookingEmailSmsHandler({ logger: loggerWithEventDetails }); + const emailsAndSmsHandler = new BookingEmailSmsHandler({ logger: tracingLogger });Also remove the now-unused import
createLoggerWithEventDetailsat Line 115 to satisfy linting.packages/lib/server/service/workflows.ts (1)
76-88: Don't maketraceContextrequired here (breaks callers).
ScheduleWorkflowRemindersArgsalready definestraceContext?: TraceContext. Making it required in this wrapper will break existing call sites (see handleConfirmation.ts). Keep it optional and pass through when present.Apply this diff:
static async scheduleWorkflowsForNewBooking({ isNormalBookingOrFirstRecurringSlot, isConfirmedByDefault, isRescheduleEvent, workflows, traceContext, ...args }: ScheduleWorkflowRemindersArgs & { isConfirmedByDefault: boolean; isRescheduleEvent: boolean; isNormalBookingOrFirstRecurringSlot: boolean; - traceContext: TraceContext; + traceContext?: TraceContext; }) {packages/lib/server/defaultResponder.ts (1)
64-65: Fix performance measure label: literal “$1” likely unintended.Use
operationfor clarity.Apply this diff:
- performance.measure(`[${ok ? "OK" : "ERROR"}][$1] ${req.method} '${req.url}'`, "Start", "End"); + performance.measure(`[${ok ? "OK" : "ERROR"}][${operation}] ${req.method} '${req.url}'`, "Start", "End");packages/features/bookings/lib/handleConfirmation.ts (4)
76-105: GuardcreateSpanwhen parenttraceContextis absent.Calling
createSpanwithundefinedmay break. Fall back to a new trace with the same metadata.Apply this diff:
- const spanContext = distributedTracing.createSpan(traceContext, "handle_confirmation", confirmationMeta); + const spanContext = traceContext + ? distributedTracing.createSpan(traceContext, "handle_confirmation", confirmationMeta) + : distributedTracing.createTrace("handle_confirmation", { meta: confirmationMeta });
108-111: PassspanContextinto EventManager.Constructor now supports tracing; propagate it for calendar event spans.
Apply this diff:
- const eventManager = new EventManager(user, apps); + const eventManager = new EventManager(user, apps, spanContext);
383-391: MissingtraceContextwhen scheduling workflow reminders.
WorkflowService.scheduleWorkflowsForNewBookingnow expects/acceptstraceContext. PassspanContextto keep the chain.Apply this diff:
await WorkflowService.scheduleWorkflowsForNewBooking({ workflows, smsReminderNumber: updatedBookings[index].smsReminderNumber, calendarEvent: evtOfBooking, hideBranding: !!updatedBookings[index].eventType?.owner?.hideBranding, isConfirmedByDefault: true, isNormalBookingOrFirstRecurringSlot: isFirstBooking, isRescheduleEvent: false, + traceContext: spanContext, });
607-609: Use the tracing logger (and fix undefinedlog).
logisn’t defined in this file, and we should leveragetracingLoggerfor consistency.Apply this diff:
- log.error("Error while scheduling workflow reminders for booking paid", safeStringify(error)); + tracingLogger.error("Error while scheduling workflow reminders for booking paid", { + error: safeStringify(error), + });packages/app-store/paypal/api/webhook.ts (1)
178-225: Bug: credentials fallback masks errors and proceeds with empty secrets
findPaymentCredentialsreturns an object with empty strings on error. Upstreamif (!foundCredentials)never triggers, and webhook verification proceeds with empty credentials. Returnnullon error and keep the upstream guard effective. Also avoid rawconsole.error.-export const findPaymentCredentials = async ( - bookingId: number -): Promise<{ clientId: string; secretKey: string; webhookId: string }> => { +export const findPaymentCredentials = async ( + bookingId: number +): Promise<{ clientId: string; secretKey: string; webhookId: string } | null> => { try { // @TODO: what about team bookings with paypal? const userFromBooking = await prisma.booking.findFirst({ @@ } catch (err) { - console.error(err); - return { - clientId: "", - secretKey: "", - webhookId: "", - }; + // Intentionally do not log secrets; let caller handle logging with trace context + return null; } };No upstream change needed beyond ensuring the existing guard remains before destructuring:
const foundCredentials = await findPaymentCredentials(booking.id); if (!foundCredentials) throw new HttpCode({ statusCode: 204, message: "No credentials found" }); const { webhookId, ...credentials } = foundCredentials;packages/features/webhooks/lib/scheduleTrigger.ts (1)
306-314: Bug: instrumented payload is computed but not used (variable shadowing)You compute
payloadwith trace injection, then re-declarepayloadinsidetry, dropping the trace. Use the previously computed value.- const payloadData = { triggerEvent, ...booking }; - const payload = JSON.stringify( - spanContext ? distributedTracing.injectTraceIntoPayload(payloadData, spanContext) : payloadData - ); + const payloadData = { triggerEvent, ...booking }; + const payload = JSON.stringify( + spanContext ? distributedTracing.injectTraceIntoPayload(payloadData, spanContext) : payloadData + ); @@ - try { - const payload = JSON.stringify({ triggerEvent, ...booking }); + try { await prisma.webhookScheduledTriggers.create({ data: { payload,packages/features/ee/payments/api/webhook.ts (1)
98-102: Use Prisma select to limit fetched fieldsFollow repo guideline: only select what you use (id, bookingId, data).
Apply this diff:
- const payment = await prisma.payment.findFirst({ - where: { - externalId: setupIntent.id, - }, - }); + const payment = await prisma.payment.findFirst({ + where: { + externalId: setupIntent.id, + }, + select: { + id: true, + bookingId: true, + data: true, + }, + });
🧹 Nitpick comments (55)
packages/features/bookings/Booker/components/hooks/useBookings.ts (2)
343-346: Makedataoptional and actually leveragetraceId(align with tracing goals).
- The cast assumes
dataalways exists; make it optional to match runtime.- You introduced
traceIdbut don’t use it. At minimum, log or surface it to help users/support correlate issues.Apply:
- const error = err as Error & { - data: { rescheduleUid: string; startTime: string; attendees: string[] }; - traceId?: string; - }; + const error = err as Error & { + data?: { rescheduleUid: string; startTime: string; attendees: string[] }; + traceId?: string; + }; + if (error.traceId) { + // Prefer tracer logger; temporarily log sanitized info + console.error("booking error traceId:", error.traceId); + }Optional: show a toast with a reference for support:
if (error.traceId) { showToast(`${t("something_went_wrong_on_our_end")} (${t("trace_reference_id")}: ${error.traceId})`, "error"); }
469-473: Surface a user-facing reference and use sanitized logging in onError.Show a toast including the trace reference id (when present) and avoid dumping the raw error (may contain PII). Prefer your tracing logger when available.
- onError: (err, _, ctx) => { - console.error("Error creating recurring booking", err); - // eslint-disable-next-line @calcom/eslint/no-scroll-into-view-embed -- It is only called when user takes an action in embed - bookerFormErrorRef && bookerFormErrorRef.current?.scrollIntoView({ behavior: "smooth" }); - }, + onError: (err) => { + const traced = err as Error & { traceId?: string }; + // eslint-disable-next-line @calcom/eslint/no-scroll-into-view-embed -- It is only called when user takes an action in embed + bookerFormErrorRef?.current?.scrollIntoView({ behavior: "smooth" }); + const msg = traced.traceId + ? `${t("something_went_wrong_on_our_end")} (${t("trace_reference_id")}: ${traced.traceId})` + : t("something_went_wrong_on_our_end"); + showToast(msg, "error"); + // Prefer distributed tracer logger; until then, log sanitized details only + console.error("Error creating recurring booking", { message: traced.message, traceId: traced.traceId }); + },Follow-up: apply the same pattern to
createBookingMutationandcreateInstantBookingMutationfor consistency.packages/features/webhooks/lib/handleWebhookScheduledTriggers.ts (2)
64-72: Create tracing before issuing fetch; attach X-Trace-Id; use tracingLogger; add a timeout.Right now the fetch is kicked off before tracing is constructed and errors are logged via
console.error. Move the request after tracing setup, add a trace header for downstream correlation, log viatracingLogger, and prevent indefinite hangs with a timeout.Apply this diff to remove the early fetch:
@@ - fetchPromises.push( - fetch(job.subscriberUrl, { - method: "POST", - body: job.payload, - headers, - }).catch((error) => { - console.error(`Webhook trigger for subscriber url ${job.subscriberUrl} failed with error: ${error}`); - }) - );Then insert this block just before cleaning the finished job:
@@ + // Add trace header if available and send request now that tracing is set up + const traceIdHeader = spanContext?.traceId ?? parsedJobPayload?._traceContext?.traceId; + if (traceIdHeader) headers["X-Trace-Id"] = traceIdHeader; + + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort("timeout"), 10_000); + fetchPromises.push( + fetch(job.subscriberUrl, { + method: "POST", + body: job.payload, + headers, + signal: controller.signal, + }) + .catch((error) => { + tracingLogger.error("Webhook trigger failed", { + subscriberUrl: job.subscriberUrl, + error, + }); + }) + .finally(() => clearTimeout(timeout)) + );Note: Consider deleting the job only after a successful dispatch (2xx) to avoid silent drops on failures.
Also applies to: 94-99
24-35: Use startAfter to compute timeSinceScheduled (and select it).
endTimeseems unrelated to scheduling latency. Usejob.startAfterwhich reflects when the job became runnable.Apply this diff:
@@ select: { id: true, jobName: true, payload: true, subscriberUrl: true, + startAfter: true, webhook: { select: { secret: true, }, }, }, @@ - tracingLogger.info("Executing scheduled webhook", { + tracingLogger.info("Executing scheduled webhook", { bookingId: parsedJobPayload?.id, triggerEvent: parsedJobPayload?.triggerEvent, subscriberUrl: job.subscriberUrl, originalTraceId: parsedJobPayload?._traceContext?.traceId, - timeSinceScheduled: parsedJobPayload?.endTime - ? Date.now() - new Date(parsedJobPayload.endTime).getTime() - : undefined, + timeSinceScheduled: Date.now() - new Date(job.startAfter).getTime(), });Also applies to: 86-93
packages/lib/tracing/error.ts (1)
8-29: Harden TracedError data handling and preserve cause/prototype.Guard against non-object error.data, set cause for better chaining, and ensure proper Error subclassing across transpilation targets.
Apply this diff:
constructor(error: unknown, traceContext: TraceContext, additionalData?: Record<string, unknown>) { const message = error instanceof Error ? error.message : String(error); super(message); + // Ensure correct prototype when targeting ES5/ES2015 + Object.setPrototypeOf(this, new.target.prototype); + this.name = error instanceof Error ? error.name : "TracedError"; this.traceId = traceContext.traceId; this.originalError = error; if (error instanceof Error && error.stack) { this.stack = error.stack; } - if (error && typeof error === "object" && "data" in error) { - const errorWithData = error as { data: Record<string, unknown> }; - this.data = { - ...errorWithData.data, - ...additionalData, - }; + if (error && typeof error === "object" && "data" in error) { + const errorWithData = error as { data?: unknown }; + const baseData = + errorWithData && typeof errorWithData.data === "object" && errorWithData.data !== null + ? (errorWithData.data as Record<string, unknown>) + : undefined; + this.data = { + ...(baseData || {}), + ...(additionalData || {}), + }; } else if (additionalData) { this.data = additionalData; } + + // Preserve error cause where supported + try { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (this as any).cause = error instanceof Error ? error : (error as unknown); + } catch { + // no-op + } }packages/features/bookings/lib/handleSeats/reschedule/owner/combineTwoSeatedBookings.ts (1)
173-173: Prefer named export over default.Named exports improve tree-shaking and refactors. Consider
export { combineTwoSeatedBookings };and updating imports across callers.packages/lib/tracing/index.ts (1)
50-67: Redact sensitive/large meta in tracing logger prefixes.Meta like request bodies can contain PII. Avoid logging them in prefixes and optionally clamp values.
Apply this diff:
getTracingLogger(context: TraceContext) { - const prefixes = [ + const prefixes = [ "distributed-trace", `trace:${context.traceId}`, `span:${context.spanId}`, `op:${context.operation}`, ]; if (context.meta) { - Object.entries(context.meta).forEach(([key, value]) => { - if (value !== undefined && value !== null) { - prefixes.push(`${key}:${value}`); - } - }); + const SENSITIVE_KEYS = new Set(["body", "authorization", "password", "token", "secret"]); + Object.entries(context.meta).forEach(([key, value]) => { + if (value === undefined || value === null) return; + if (SENSITIVE_KEYS.has(key.toLowerCase())) return; + const str = String(value); + const trimmed = str.length > 200 ? `${str.slice(0, 200)}…` : str; + prefixes.push(`${key}:${trimmed}`); + }); } return this.loggerInstance.getSubLogger({ prefix: prefixes }); }packages/lib/server/defaultResponder.test.ts (3)
10-22: Add assertion for X-Trace-Id header.Since defaultResponder sets X-Trace-Id, assert it in tests.
Apply this diff:
await defaultResponder(f)(req, res); expect(res.json).toHaveBeenCalled(); + expect(res.setHeader).toHaveBeenCalledWith("X-Trace-Id", expect.any(String));
35-45: Also assert header in error case (NoAvailableUsersFound).Apply this diff:
await defaultResponder(f)(req, res); expect(res.status).toHaveBeenCalledWith(409); + expect(res.setHeader).toHaveBeenCalledWith("X-Trace-Id", expect.any(String));
45-56: Also assert header in rate limit case.Apply this diff:
await defaultResponder(f)(req, res); expect(res.status).toHaveBeenCalledWith(429); + expect(res.setHeader).toHaveBeenCalledWith("X-Trace-Id", expect.any(String));packages/features/bookings/lib/handleBookingRequested.ts (1)
10-10: TraceContext added but never used; propagate it to webhooks and workflows.Forward the trace to downstream operations for continuity and better debugging.
Apply:
@@ -import { type TraceContext } from "@calcom/lib/tracing"; +import { type TraceContext } from "@calcom/lib/tracing"; +import { distributedTracing } from "@calcom/lib/tracing/factory"; @@ - const webhookPayload = getWebhookPayloadForBooking({ + const rawWebhookPayload = getWebhookPayloadForBooking({ booking, evt, }); + const webhookPayload = distributedTracing.injectTraceIntoPayload(rawWebhookPayload, args.traceContext); @@ - await WorkflowService.scheduleWorkflowsFilteredByTriggerEvent({ + await WorkflowService.scheduleWorkflowsFilteredByTriggerEvent({ workflows, smsReminderNumber: booking.smsReminderNumber, hideBranding: !!booking.eventType?.owner?.hideBranding, calendarEvent: { ...evt, @@ }, triggers: [WorkflowTriggerEvents.BOOKING_REQUESTED], + traceContext: args.traceContext, });Also applies to: 58-59
packages/features/bookings/lib/handleWebhookTrigger.ts (2)
16-17: Make traceContext optional or remove fallback.You typed traceContext as required but still branch on its absence. Either make it optional or drop the fallback.
Apply:
- traceContext: TraceContext; + traceContext?: TraceContext;No other code changes needed since you already handle the optional case.
Also applies to: 21-31
36-46: Log non-Event payload failures too.Currently errors for non-Event payloads are swallowed. Add a generic log path to aid debugging.
(e) => { if (isEventPayload(args.webhookData)) { tracingLogger.error( `Error executing webhook for event: ${args.eventTrigger}, URL: ${sub.subscriberUrl}, booking id: ${args.webhookData.bookingId}, booking uid: ${args.webhookData.uid}`, safeStringify(e) ); + } else { + tracingLogger.error( + `Error executing webhook for event: ${args.eventTrigger}, URL: ${sub.subscriberUrl}`, + safeStringify(e) + ); } }packages/features/ee/workflows/lib/reminders/reminderScheduler.ts (1)
16-16: traceContext added but unused; either propagate or drop until needed.To keep signal continuity, consider creating a span per workflow step and/or passing traceContext into downstream schedulers. Otherwise, remove the arg for now to avoid API surface bloat.
Example minimal propagation (only if providers accept it):
-const processWorkflowStep = async ( +const processWorkflowStep = async ( workflow: Workflow, step: WorkflowStep, { smsReminderNumber, calendarEvent, emailAttendeeSendToOverride, hideBranding, seatReferenceUid, - }: ProcessWorkflowStepParams + }: ProcessWorkflowStepParams, + traceContext?: TraceContext ) => { @@ - await scheduleSMSReminder({ + await scheduleSMSReminder({ evt, @@ - }); + traceContext, + }); @@ - await scheduleEmailReminder({ + await scheduleEmailReminder({ evt, @@ - }); + traceContext, + }); @@ - await scheduleWhatsappReminder({ + await scheduleWhatsappReminder({ evt, @@ - }); + traceContext, + });And thread args.traceContext through calls to processWorkflowStep in _scheduleWorkflowReminders/_sendCancelledReminders.
Also applies to: 51-52
packages/app-store/btcpayserver/api/webhook.ts (1)
62-65: Header lookup: Node lowercases header names.Accessing both "btcpay-sig" and "BTCPay-Sig" is redundant. Use the lowercase key only.
- const signature = req.headers["btcpay-sig"] || req.headers["BTCPay-Sig"]; + const signature = req.headers["btcpay-sig"];packages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.ts (1)
67-86: Consider propagating trace metadata to async tasks.To keep end-to-end traceability when tasks execute later, consider including a trace reference (e.g., traceId/spanId) in the task payload/options if Tasker supports it.
Also applies to: 97-118
packages/app-store/hitpay/api/webhook.ts (2)
47-58: Initialize trace per request and return X-Trace-Id header.Good trace initialization. Recommend setting a response header for client correlation.
Apply:
const traceContext = distributedTracing.createTrace("hitpay_webhook_handler", { meta: webhookMeta, }); const tracingLogger = distributedTracing.getTracingLogger(traceContext); + // Surface trace id for correlation + res.setHeader("X-Trace-Id", traceContext.traceId);
68-90: Scope Prisma select to only needed fields.Avoid fetching unnecessary columns; explicitly select
credentials.key.Apply:
const payment = await prisma.payment.findFirst({ where: { externalId: obj.payment_request_id, }, select: { id: true, amount: true, bookingId: true, booking: { select: { user: { select: { - credentials: { - where: { - type: "hitpay_payment", - }, - }, + credentials: { + where: { type: "hitpay_payment" }, + select: { key: true }, + }, }, }, }, }, }, });packages/app-store/alby/api/webhook.ts (3)
21-32: Per-request trace looks good; add X-Trace-Id header.Apply:
const traceContext = distributedTracing.createTrace("alby_webhook_handler", { meta: webhookMeta, }); const tracingLogger = distributedTracing.getTracingLogger(traceContext); + res.setHeader("X-Trace-Id", traceContext.traceId);
42-46: Replace console.error with tracingLogger for consistency.Apply:
- console.error(parseHeaders.error); + tracingLogger.warn("Invalid webhook headers", { error: parseHeaders.error?.message }); ... - console.error(parse.error); + tracingLogger.warn("Invalid webhook payload", { error: parse.error?.message });Also applies to: 51-54
62-84: Limit Prisma select to credentials.key only.Apply:
const payment = await prisma.payment.findFirst({ where: { uid: parsedPayload.metadata.payer_data.referenceId, }, select: { id: true, amount: true, bookingId: true, booking: { select: { user: { select: { - credentials: { - where: { - type: "alby_payment", - }, - }, + credentials: { + where: { type: "alby_payment" }, + select: { key: true }, + }, }, }, }, }, }, });packages/features/bookings/lib/handleSeats/reschedule/attendee/attendeeRescheduleSeatedBooking.ts (1)
15-22: Unused traceContext parameter.Either use it (create a local span/logs) or underscore to silence lint until used.
Option A (minimal):
- traceContext: TraceContext + _traceContext: TraceContextOption B (instrumentation):
+import { distributedTracing } from "@calcom/lib/tracing/factory"; ... eventManager: EventManager, - traceContext: TraceContext + traceContext: TraceContext ) => { + const spanContext = distributedTracing.createSpan(traceContext, "attendee_reschedule_seated_booking"); + const tracingLogger = distributedTracing.getTracingLogger(spanContext); + tracingLogger.info("Attendee reschedule started", { newBookingId: newTimeSlotBooking?.id });packages/features/bookings/lib/handleSeats/reschedule/owner/ownerRescheduleSeatedBooking.ts (1)
56-56: Prefer named export over default exportPromotes clearer imports/tree-shaking. Optional, can be follow-up.
Apply if/when convenient:
-const ownerRescheduleSeatedBooking = async ( +export const ownerRescheduleSeatedBooking = async ( /* ... */ ) => { /* ... */ }; - -export default ownerRescheduleSeatedBooking;packages/lib/server/getServerErrorFromUnknown.ts (1)
50-57: Consider keeping original TRPC cause for parityOptional: include
causefor consistency with other branches.- return new HttpError({ - statusCode, - message: cause.message, - data: traceId ? { ...tracedData, traceId } : undefined, - }); + return new HttpError({ + statusCode, + message: cause.message, + cause, + data: traceId ? { ...(tracedData ?? {}), traceId } : undefined, + });packages/features/ee/workflows/lib/reminders/scheduleMandatoryReminder.ts (2)
96-102: Log structured error objects; avoid stringifyingPre-stringifying loses structure and may double encode.
- } catch (error) { - tracingLogger.error("Error while scheduling mandatory reminders", JSON.stringify({ error })); - } + } catch (error) { + tracingLogger.error("Error while scheduling mandatory reminders", { + error: error instanceof Error ? { message: error.message, stack: error.stack } : String(error), + }); + }
100-102: Ditto for outer catchKeep logs structured and consistent.
- } catch (error) { - tracingLogger.error("Error while scheduling mandatory reminders", JSON.stringify({ error })); - } + } catch (error) { + tracingLogger.error("Error while scheduling mandatory reminders", { + error: error instanceof Error ? { message: error.message, stack: error.stack } : String(error), + }); + }packages/features/bookings/lib/handleSeats/reschedule/owner/moveSeatedBookingToNewTimeSlot.ts (3)
82-83: Log structured error/results; avoid stringifyingPreserve structure for observability.
- tracingLogger.error(`Booking ${organizerUser.name} failed`, JSON.stringify({ error, results })); + tracingLogger.error(`Booking ${organizerUser.name} failed`, { error, results });
100-103: Prefer structured debug logConsistent with above.
- tracingLogger.debug("Emails: Sending reschedule emails - handleSeats"); + tracingLogger.debug("Emails: Sending reschedule emails - handleSeats", { + bookingId: seatedBooking.id, + });
116-116: Prefer named export over default exportOptional follow-up for consistency and tree-shaking.
-export default moveSeatedBookingToNewTimeSlot; +export { moveSeatedBookingToNewTimeSlot };Note: adjust imports at call sites accordingly.
packages/trpc/server/routers/viewer/bookings/confirm.handler.ts (1)
398-399: Use tracing logger instead of console in catchSurface failures with trace correlation.
- await handleWebhookTrigger({ subscriberOptions, eventTrigger, webhookData, traceContext }); + await handleWebhookTrigger({ subscriberOptions, eventTrigger, webhookData, traceContext }); @@ - } catch (error) { - // Silently fail - console.error( - "Error while scheduling workflow reminders for BOOKING_REJECTED:", - error instanceof Error ? error.message : String(error) - ); - } + } catch (error) { + // Silently fail but capture with trace + distributedTracing + .getTracingLogger(traceContext) + .error("Error while scheduling workflow reminders for BOOKING_REJECTED", { + error: error instanceof Error ? { message: error.message, stack: error.stack } : String(error), + }); + }Also applies to: 416-421
packages/features/bookings/lib/EventManager.ts (2)
294-304: Ensure span lifecycle and keep all logs within the traceYou create a span and a tracing logger, but:
- The span is never explicitly ended.
- Subsequent logs in this function still use the module-level
log, losing the trace context.Recommend scoping a local
logto the tracing logger and (if your tracing API requires it) end the span in a finally block.public async create(event: CalendarEvent): Promise<CreateUpdateResult> { const spanContext = this.traceContext ? distributedTracing.createSpan(this.traceContext, "calendar_event_creation") : undefined; - const tracingLogger = spanContext ? distributedTracing.getTracingLogger(spanContext) : log; + const tracingLogger = spanContext ? distributedTracing.getTracingLogger(spanContext) : log; + // Use the tracing-aware logger for all logs in this method + const log = tracingLogger; - tracingLogger.info("EventManager.create started", { + log.info("EventManager.create started", { eventTitle: event.title, attendeeCount: event.attendees.length, integrationCount: this.calendarCredentials.length, }); + // Optionally ensure span is ended if your API requires it: + // try { ...existing logic... } finally { if (spanContext) distributedTracing.endSpan(spanContext); }Please confirm the correct API for ending a span (e.g.,
endSpan,finishSpan, etc.). If your implementation does not require explicit ending, disregard the finally suggestion.
147-152: Trace context stored but not leveraged uniformly across other operationsConstructor accepts and stores
traceContext, but onlycreate()uses it. For parity, consider instrumenting other mutating flows (reschedule,updateLocation,deleteEventsAndMeetings, etc.) with child spans/logging.Also applies to: 181-181
packages/features/bookings/lib/handleSeats/reschedule/rescheduleSeatedBooking.ts (1)
6-8: Remove unused import
distributedTracingis imported but not used.-import { distributedTracing } from "@calcom/lib/tracing/factory";packages/features/bookings/lib/handleSeats/handleSeats.ts (2)
45-51: Avoid placing raw user info in trace metadata
userInfo: JSON.stringify(reqBodyUser)can contain PII. Prefer redacting or omitting user details from trace meta.- const seatsMeta = { + const seatsMeta = { eventTypeId: eventType.id.toString(), - userInfo: JSON.stringify(reqBodyUser) || "null", + // Avoid PII in trace meta + userInfo: typeof reqBodyUser === "string" ? "user_ref" : "user_ref", eventTypeSlug: eventType.slug || "unknown", bookingUid: reqBookingUid || "null", rescheduleUid: rescheduleUid || "null", };
53-58: End the span if required by your tracing APIYou create a span/trace but never end it. Wrap the body in try/finally and end/finish the span if applicable.
- const spanContext = traceContext + const spanContext = traceContext ? distributedTracing.createSpan(traceContext, "handle_seats", seatsMeta) : distributedTracing.createTrace("handle_seats_fallback", { meta: seatsMeta, }); - const tracingLogger = distributedTracing.getTracingLogger(spanContext); + const tracingLogger = distributedTracing.getTracingLogger(spanContext); + // try { ... } finally { distributedTracing.endSpan?.(spanContext); }packages/features/bookings/lib/handleNewBooking.ts (4)
1178-1180: Redact guest emails in logs
guestsRemovedwill contain emails. Avoid logging raw emails.- tracingLogger.info("Removed guests from the booking", guestsRemoved); + tracingLogger.info("Removed guests from the booking", { count: guestsRemoved.length });
1702-1703: Propagate trace to EventManager (original host) during organizer-change deletionTo keep deletion operations in the same trace, pass
traceContextto the EventManager constructed for the original host.- const originalHostEventManager = new EventManager( - { ...originalRescheduledBooking.user, credentials: refreshedOriginalHostCredentials }, - apps - ); + const originalHostEventManager = new EventManager( + { ...originalRescheduledBooking.user, credentials: refreshedOriginalHostCredentials }, + apps, + traceContext + );
652-676: Be mindful of PII in logsThe structured log includes
reqBody.userand other request details. Consider using a PII-free projection for user fields.
2268-2269: End/flush trace if applicableYou create/update a trace at the start of the handler but never close it. If your tracer requires explicit finishing/flushing, add it here (or in a finally). Otherwise, ignore.
packages/lib/server/defaultResponder.ts (1)
36-44: Set X-Trace-Id early to cover handlers that write their own response.If the handler writes directly and ends the response, the header may be missing. Set it before invoking the handler.
Apply this diff:
const tracedReq = req as TracedRequest; tracedReq.traceContext = traceContext; + // Ensure trace id is always present, even if handler writes directly. + if (!res.headersSent) { + res.setHeader("X-Trace-Id", traceContext.traceId); + }packages/features/bookings/lib/handleNewBooking/logger.ts (1)
9-17: Optional: preserve richer metadata values.Using
.toString()turns objects/arrays into “[object Object]”. Consider JSON-stringifying non-strings and truncating.Apply this diff:
- const stringMeta: Record<string, string> = Object.fromEntries( - Object.entries(eventMeta).map(([key, value]) => [key, value?.toString() || "null"]) - ); + const stringMeta: Record<string, string> = Object.fromEntries( + Object.entries(eventMeta).map(([key, value]) => { + if (value == null) return [key, "null"]; + if (typeof value === "string") return [key, value]; + try { + return [key, JSON.stringify(value).slice(0, 1024)]; + } catch { + return [key, String(value)]; + } + }) + );packages/app-store/paypal/api/webhook.ts (2)
84-89: Expose trace id to clients via response headerSet
X-Trace-Idso the trace can be correlated externally (e.g., in PayPal portal or logs).const traceContext = distributedTracing.createTrace("paypal_webhook_handler", { meta: webhookMeta, }); const tracingLogger = distributedTracing.getTracingLogger(traceContext); + // Expose trace id for correlation + res.setHeader("X-Trace-Id", traceContext.traceId);
101-108: Use tracing logger instead of console.error (keeps logs correlated with trace)Replace
console.errorwithtracingLogger.errorfor structured, correlated logs.- if (!parseHeaders.success) { - console.error(parseHeaders.error); + if (!parseHeaders.success) { + tracingLogger.error("Invalid PayPal webhook headers", { + error: parseHeaders.error, + }); throw new HttpCode({ statusCode: 400, message: "Bad Request" }); } - const parse = eventSchema.safeParse(JSON.parse(bodyAsString)); - if (!parse.success) { - console.error(parse.error); + const parse = eventSchema.safeParse(JSON.parse(bodyAsString)); + if (!parse.success) { + tracingLogger.error("Invalid PayPal webhook payload", { + error: parse.error, + }); throw new HttpCode({ statusCode: 400, message: "Bad Request" }); }packages/features/bookings/lib/handleNewBooking/validateEventLength.ts (1)
25-41: Ensure meta values are strings (avoid undefined in meta payload)
JSON.stringify(eventTypeMultipleDuration)can yieldundefined. Coerce to a string to satisfyRecord<string, string>.- const spanContext = traceContext - ? distributedTracing.createSpan(traceContext, "validate_event_length", { - reqBodyStart, - reqBodyEnd, - eventTypeMultipleDuration: JSON.stringify(eventTypeMultipleDuration), - eventTypeLength: eventTypeLength.toString(), - }) - : distributedTracing.createTrace("validate_event_length_fallback", { - meta: { - reqBodyStart, - reqBodyEnd, - eventTypeMultipleDuration: JSON.stringify(eventTypeMultipleDuration), - eventTypeLength: eventTypeLength.toString(), - }, - }); + const spanContext = traceContext + ? distributedTracing.createSpan(traceContext, "validate_event_length", { + reqBodyStart, + reqBodyEnd, + eventTypeMultipleDuration: JSON.stringify(eventTypeMultipleDuration ?? []), + eventTypeLength: String(eventTypeLength), + }) + : distributedTracing.createTrace("validate_event_length_fallback", { + meta: { + reqBodyStart, + reqBodyEnd, + eventTypeMultipleDuration: JSON.stringify(eventTypeMultipleDuration ?? []), + eventTypeLength: String(eventTypeLength), + }, + });packages/features/bookings/lib/handleNewBooking/ensureAvailableUsers.ts (2)
206-213: Prefer warn for expected validation failures to reduce noiseThese are common, non-exceptional conditions. Consider
warnto keep error logs actionable.- tracingLogger.error(`Booking outside restriction schedule availability.`, piiFreeInputDataForLogging); + tracingLogger.warn(`Booking outside restriction schedule availability.`, piiFreeInputDataForLogging);
220-223: Avoid logging large blobs at debug without bounds
JSON.stringify({ bufferedBusyTimes, dateRanges, ... })can be large. Consider truncating or summarizing to avoid log bloat.packages/features/bookings/lib/handleNewBooking/validateBookingTimeIsNotOutOfBounds.ts (1)
55-56: Prefer structured logging over stringified Error
JSON.stringify(error)often yields{}. Log fields explicitly for better observability.- traceLogger.info(`Booking eventType ${eventType.id} failed`, JSON.stringify({ error })); + traceLogger.info("Booking validation failed (date in past)", { + eventTypeId: eventType.id, + message: error.message, + name: error.name, + });packages/features/webhooks/lib/scheduleTrigger.ts (3)
293-305: Create span meta and resolve TODOAttach minimal meta (bookingId, triggerEvent) to the span; also resolve the TODO or track it.
- // TODO: check this - const spanContext = traceContext - ? distributedTracing.createSpan(traceContext, "webhook_scheduling") - : undefined; + // Span for scheduling; attaches key identifiers + const spanContext = traceContext + ? distributedTracing.createSpan(traceContext, "webhook_scheduling", { + bookingId: String(booking.id), + triggerEvent, + }) + : undefined;Happy to open a follow-up issue to track remaining TODOs.
333-334: Use tracer-aware logger instead of console.error and fix messagePrefer
tracingLogger.error(orlog.errorwhen no trace) and correct the message to reflect scheduling, not cancelling.- console.error("Error cancelling scheduled jobs", error); + if (spanContext) { + distributedTracing.getTracingLogger(spanContext).error("Error scheduling webhook trigger", { error }); + } else { + log.error("Error scheduling webhook trigger", safeStringify(error)); + }
74-95: Prisma: replace include with select to limit fetched dataPer guidelines, avoid
include. Refactor toselectonly necessary fields.- const bookings = await prisma.booking.findMany({ + const bookings = await prisma.booking.findMany({ where: { ...where, startTime: { gte: new Date(), }, status: BookingStatus.ACCEPTED, }, - include: { - eventType: { - select: { - bookingFields: true, - }, - }, - attendees: { - select: { - name: true, - email: true, - }, - }, - }, + select: { + id: true, + title: true, + description: true, + customInputs: true, + responses: true, + startTime: true, + endTime: true, + location: true, + status: true, + eventType: { + select: { + bookingFields: true, + }, + }, + attendees: { + select: { + name: true, + email: true, + }, + }, + }, });packages/features/ee/payments/api/webhook.ts (5)
55-63: Add idempotency guard to avoid reprocessing already successful paymentsSelect the success flag and short-circuit if already processed.
Apply this diff:
const payment = await prisma.payment.findFirst({ where: { externalId: paymentIntent.id, }, select: { id: true, bookingId: true, + success: true, }, }); + if (payment?.success) { + tracingLogger.info("Stripe: Payment already processed", { + paymentId: payment.id, + bookingId: payment.bookingId, + paymentIntentId: paymentIntent.id, + }); + return; + }
134-141: Propagate trace to EventManager (if supported)To keep spans connected, pass spanContext to EventManager constructor if it accepts it.
Proposed change:
- const eventManager = new EventManager({ ...user, credentials: allCredentials }, metadata?.apps); + const eventManager = new EventManager( + { ...user, credentials: allCredentials }, + metadata?.apps, + spanContext + );Please confirm EventManager’s constructor supports an optional TraceContext as the third parameter.
192-203: Return X-Trace-Id header for clientsExpose traceId for correlation in responses.
Apply this diff:
const traceContext = distributedTracing.createTrace("stripe_webhook_handler", { meta: webhookMeta, }); const tracingLogger = distributedTracing.getTracingLogger(traceContext); + res.setHeader("X-Trace-Id", traceContext.traceId);
208-219: Guard stripe-signature header typeconstructEvent expects a string; enforce type to avoid runtime errors if header is an array.
Apply this diff:
- const sig = req.headers["stripe-signature"]; - if (!sig) { + const sigHeader = req.headers["stripe-signature"]; + if (!sigHeader || typeof sigHeader !== "string") { throw new HttpCode({ statusCode: 400, message: "Missing stripe-signature" }); } @@ - const event = stripe.webhooks.constructEvent(payload, sig, process.env.STRIPE_WEBHOOK_SECRET); + const event = stripe.webhooks.constructEvent(payload, sigHeader, process.env.STRIPE_WEBHOOK_SECRET);
245-249: Use log level based on statusCode; avoid error logs for 2xx/4xx control-flowPrevents noisy “errors” for 202/204 paths and keeps severity meaningful.
Apply this diff:
- tracingLogger.error("Webhook Error", { - message: err.message, - statusCode: err.statusCode, - stack: IS_PRODUCTION ? undefined : err.stack, - }); + const statusCode = err instanceof HttpCode ? err.statusCode : 500; + const logMethod = statusCode >= 500 ? "error" : statusCode >= 400 ? "warn" : "info"; + tracingLogger[logMethod]("Webhook Handler Result", { + message: err.message, + statusCode, + stack: IS_PRODUCTION ? undefined : err.stack, + });
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/features/bookings/lib/handleSeats/handleSeats.ts (1)
88-92: Replaceincludewithselectin Prisma query.The query uses
include: { bookingSeat: true }which violates the coding guideline requiringselectinstead ofincludefor Prisma queries.As per coding guidelines
Apply this diff to fix the issue:
select: { uid: true, id: true, - attendees: { include: { bookingSeat: true } }, + attendees: { + select: { + id: true, + email: true, + name: true, + timeZone: true, + locale: true, + bookingSeat: { + select: { + id: true, + referenceUid: true, + bookingId: true, + attendeeId: true, + data: true, + } + } + } + }, userId: true,Note: Adjust the selected fields in
bookingSeatbased on what's actually needed downstream in the code.packages/features/ee/payments/api/webhook.ts (1)
133-140: Pass spanContext to EventManager for trace continuity.The
EventManageris instantiated without thespanContextparameter at line 134. Based on the changes in this PR (see line 8 import and the pattern used inhandlePaymentSuccessfrom relevant code snippets),EventManagershould receive the span context to maintain distributed tracing continuity.Apply this diff:
if (!requiresConfirmation) { - const eventManager = new EventManager({ ...user, credentials: allCredentials }, metadata?.apps); + const eventManager = new EventManager({ ...user, credentials: allCredentials }, metadata?.apps, spanContext); const scheduleResult = areCalendarEventsEnabled ? await eventManager.create(evt) : placeholderCreatedEvent;
♻️ Duplicate comments (3)
packages/app-store/_utils/payments/handlePaymentSuccess.ts (1)
19-33: Allow starting a trace when none is provided.Requiring callers to pass a
TraceContextleaves no path for fresh payment-success flows (queue jobs, tests, legacy callers) to emit traces; they’ll either skip tracing entirely or fail at runtime. Please keep the parameter optional and fall back to creating a new trace, as previously suggested.-export async function handlePaymentSuccess(paymentId: number, bookingId: number, traceContext: TraceContext) { +export async function handlePaymentSuccess( + paymentId: number, + bookingId: number, + traceContext?: TraceContext +) { const paymentMeta = { paymentId: paymentId.toString(), bookingId: bookingId.toString(), }; - const spanContext = distributedTracing.createSpan(traceContext, "payment_success_processing", paymentMeta); + const spanContext = traceContext + ? distributedTracing.createSpan(traceContext, "payment_success_processing", paymentMeta) + : distributedTracing.createTrace("payment_success_processing", { meta: paymentMeta }); const tracingLogger = distributedTracing.getTracingLogger(spanContext); tracingLogger.info("Processing payment success", { paymentId, bookingId, - originalTraceId: traceContext.traceId, + traceId: spanContext.traceId, });packages/features/ee/payments/api/webhook.ts (1)
63-71: Remove the redundant duplicate check.Line 71 contains a duplicate
if (!payment?.bookingId)check that is already handled in lines 63-70. This is dead code and should be removed.Apply this diff:
if (!payment?.bookingId) { tracingLogger.error("Stripe: Payment Not Found", { paymentIntentId: paymentIntent.id, paymentId: payment?.id, bookingId: payment?.bookingId, }); throw new HttpCode({ statusCode: 204, message: "Payment not found" }); } - if (!payment?.bookingId) throw new HttpCode({ statusCode: 204, message: "Payment not found" });packages/features/bookings/lib/handleNewBooking/ensureAvailableUsers.ts (1)
61-61: Parameter change from Logger to TraceContext is correct.The internal parameter change from
loggerWithEventDetails: Logger<unknown>totraceContext: TraceContextis correct for the distributed tracing refactor. The public signature remains unchanged due to thewithReportingwrapper.Note: A past review comment already flagged that a caller at
packages/trpc/server/routers/viewer/teams/roundRobin/getRoundRobinHostsToReasign.handler.ts:141is passing a Logger instead of TraceContext. That issue should be addressed separately.
🧹 Nitpick comments (2)
packages/features/ee/round-robin/roundRobinReassignment.ts (1)
399-402: Add error handling for event deletion.The
deleteEventsAndMeetingscall lacks error handling. If deletion fails (e.g., due to network issues, credential problems, or calendar API errors), the system could proceed to create new events, leaving orphaned calendar entries for the original organizer.Consider wrapping the deletion in a try-catch block and logging failures:
+ try { await originalHostEventManager.deleteEventsAndMeetings({ event: deletionEvent, bookingReferences: booking.references, }); + } catch (error) { + roundRobinReassignLogger.error( + `Failed to delete events for original organizer ${originalOrganizer.id}`, + error + ); + // Consider whether to throw or continue based on business requirements + }packages/features/bookings/lib/handleConfirmation.ts (1)
19-19: Remove unused logger import.After replacing all
loggercalls withtracingLogger, this import is no longer used and should be removed.Apply this diff to remove the unused import:
-import logger from "@calcom/lib/logger";
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
apps/web/public/static/locales/en/common.json(1 hunks)apps/web/test/lib/confirm.handler.test.ts(2 hunks)packages/app-store/_utils/payments/handlePaymentSuccess.ts(4 hunks)packages/features/bookings/Booker/components/hooks/useBookings.ts(2 hunks)packages/features/bookings/lib/BookingEmailSmsHandler.ts(1 hunks)packages/features/bookings/lib/handleConfirmation.ts(12 hunks)packages/features/bookings/lib/handleNewBooking.ts(48 hunks)packages/features/bookings/lib/handleNewBooking/ensureAvailableUsers.ts(8 hunks)packages/features/bookings/lib/handleSeats/handleSeats.ts(6 hunks)packages/features/bookings/lib/handleWebhookTrigger.ts(3 hunks)packages/features/ee/payments/api/webhook.ts(7 hunks)packages/features/ee/round-robin/roundRobinReassignment.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/features/bookings/lib/BookingEmailSmsHandler.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/web/public/static/locales/en/common.json
- packages/features/bookings/lib/handleWebhookTrigger.ts
- packages/features/bookings/Booker/components/hooks/useBookings.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
apps/web/test/lib/confirm.handler.test.tspackages/features/bookings/lib/handleConfirmation.tspackages/features/bookings/lib/handleSeats/handleSeats.tspackages/features/ee/round-robin/roundRobinReassignment.tspackages/features/bookings/lib/handleNewBooking/ensureAvailableUsers.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/ee/payments/api/webhook.tspackages/app-store/_utils/payments/handlePaymentSuccess.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/web/test/lib/confirm.handler.test.tspackages/features/bookings/lib/handleConfirmation.tspackages/features/bookings/lib/handleSeats/handleSeats.tspackages/features/ee/round-robin/roundRobinReassignment.tspackages/features/bookings/lib/handleNewBooking/ensureAvailableUsers.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/ee/payments/api/webhook.tspackages/app-store/_utils/payments/handlePaymentSuccess.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
apps/web/test/lib/confirm.handler.test.tspackages/features/bookings/lib/handleConfirmation.tspackages/features/bookings/lib/handleSeats/handleSeats.tspackages/features/ee/round-robin/roundRobinReassignment.tspackages/features/bookings/lib/handleNewBooking/ensureAvailableUsers.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/ee/payments/api/webhook.tspackages/app-store/_utils/payments/handlePaymentSuccess.ts
🧠 Learnings (2)
📚 Learning: 2025-08-21T13:55:23.470Z
Learnt from: alishaz-polymath
PR: calcom/cal.com#23247
File: packages/features/webhooks/lib/service/FormWebhookService.ts:0-0
Timestamp: 2025-08-21T13:55:23.470Z
Learning: In the new webhook architecture for Cal.com, schedulePayload is considered legacy code that doesn't fit the clean architecture. The new architecture keeps webhook scheduling logic within the Service layer, specifically through a new method WebhookService.scheduleDelayedWebhooks, rather than using the old centralized schedulePayload helper.
Applied to files:
packages/features/bookings/lib/handleConfirmation.ts
📚 Learning: 2025-09-03T09:52:51.182Z
Learnt from: hariombalhara
PR: calcom/cal.com#23541
File: packages/features/bookings/lib/di/modules/RegularBookingServiceModule.ts:22-28
Timestamp: 2025-09-03T09:52:51.182Z
Learning: The IBookingServiceDependencies interface in packages/features/bookings/lib/handleNewBooking.ts contains 6 properties: cacheService, checkBookingAndDurationLimitsService, prismaClient, bookingRepository, featuresRepository, and checkBookingLimitsService. This interface is used by RegularBookingService and matches the depsMap structure in RegularBookingServiceModule.
Applied to files:
packages/features/bookings/lib/handleNewBooking.ts
🧬 Code graph analysis (8)
apps/web/test/lib/confirm.handler.test.ts (1)
packages/lib/tracing/factory.ts (1)
distributedTracing(14-14)
packages/features/bookings/lib/handleConfirmation.ts (2)
packages/lib/tracing/index.ts (1)
TraceContext(3-10)packages/lib/tracing/factory.ts (1)
distributedTracing(14-14)
packages/features/bookings/lib/handleSeats/handleSeats.ts (4)
packages/features/bookings/lib/handleSeats/types.d.ts (1)
NewSeatedBookingObject(25-61)packages/lib/tracing/index.ts (1)
TraceContext(3-10)packages/lib/tracing/factory.ts (1)
distributedTracing(14-14)packages/features/bookings/lib/handleWebhookTrigger.ts (1)
handleWebhookTrigger(53-53)
packages/features/ee/round-robin/roundRobinReassignment.ts (2)
packages/lib/tracing/factory.ts (1)
distributedTracing(14-14)packages/features/bookings/lib/handleNewBooking/ensureAvailableUsers.ts (1)
ensureAvailableUsers(260-260)
packages/features/bookings/lib/handleNewBooking/ensureAvailableUsers.ts (2)
packages/lib/tracing/index.ts (1)
TraceContext(3-10)packages/lib/tracing/factory.ts (1)
distributedTracing(14-14)
packages/features/bookings/lib/handleNewBooking.ts (5)
packages/lib/tracing/index.ts (1)
TraceContext(3-10)packages/lib/tracing/factory.ts (1)
distributedTracing(14-14)packages/features/bookings/lib/BookingEmailSmsHandler.ts (1)
BookingEmailSmsHandler(95-325)packages/lib/http-error.ts (1)
HttpError(1-45)packages/features/bookings/lib/EventManager.ts (1)
EventManager(136-1328)
packages/features/ee/payments/api/webhook.ts (3)
packages/lib/tracing/index.ts (1)
TraceContext(3-10)packages/lib/tracing/factory.ts (1)
distributedTracing(14-14)packages/app-store/_utils/payments/handlePaymentSuccess.ts (1)
handlePaymentSuccess(19-135)
packages/app-store/_utils/payments/handlePaymentSuccess.ts (4)
packages/lib/tracing/index.ts (1)
TraceContext(3-10)packages/lib/tracing/factory.ts (1)
distributedTracing(14-14)packages/features/bookings/lib/EventManager.ts (1)
EventManager(136-1328)packages/features/bookings/lib/handleBookingRequested.ts (1)
handleBookingRequested(22-128)
🔇 Additional comments (31)
packages/features/bookings/lib/handleSeats/handleSeats.ts (6)
8-9: LGTM!The tracing imports are correctly added and align with the distributed tracing infrastructure.
18-20: LGTM!The function signature correctly extends
NewSeatedBookingObjectwith an optionaltraceContextparameter, maintaining backward compatibility while enabling distributed tracing.
45-70: LGTM!The trace context initialization is well-structured:
- Comprehensive metadata collection in
seatsMeta- Proper conditional logic for span vs. trace creation
- Informative initial log with relevant booking context
123-129: LGTM!The
spanContextis correctly propagated torescheduleSeatedBooking, maintaining the trace across the reschedule flow.
141-167: LGTM!The workflow scheduling correctly propagates the
spanContextastraceContextand uses thetracingLoggerfor error logging, ensuring proper trace continuity.
189-195: LGTM!The webhook trigger correctly receives the
spanContextastraceContext, ensuring the distributed trace propagates through the webhook flow.packages/features/ee/payments/api/webhook.ts (3)
73-73: Verify handlePaymentSuccess doesn't throw 2xx status codes.Based on the relevant code snippets,
handlePaymentSuccessthrowsnew HttpCode({ statusCode: 200 })at the end of successful processing. This would be caught by the catch block (lines 242-254) and logged as an error, which is incorrect for successful flows.The
handlePaymentSuccessfunction should return normally instead of throwing a 200 status code. Consider updatingpackages/app-store/_utils/payments/handlePaymentSuccess.tsto return a success result rather than throwing:// Instead of: throw new HttpCode({ statusCode: 200, message: `Booking with id '${booking.id}' was paid and confirmed.` }); // Do: return { success: true, bookingId: booking.id, message: `Booking with id '${booking.id}' was paid and confirmed.` };Based on previous review comments.
178-178: LGTM!The
WebhookHandlertype correctly reflects the updated signature with optionaltraceContextparameter, maintaining consistency across all webhook handlers.
191-258: LGTM!The webhook handler correctly implements distributed tracing at the entry point:
- Creates a trace context with relevant metadata
- Propagates the trace context to downstream handlers
- Uses the tracing logger for all log statements
- Conditionally includes stack traces in error logs based on environment
The implementation maintains trace continuity throughout the webhook processing flow.
packages/features/ee/round-robin/roundRobinReassignment.ts (1)
151-166: LGTM! Trace context integration looks correct.The distributed tracing integration is well-implemented:
- Creates a properly named trace context with relevant metadata
- Provides a safe fallback for
bookingUid- Correctly passes the context to
ensureAvailableUsers, which is wrapped withwithReportingfor trace propagationpackages/features/bookings/lib/handleNewBooking/ensureAvailableUsers.ts (4)
6-6: LGTM! Import reordering and new tracing imports.The import changes correctly support the distributed tracing refactor. No issues.
Also applies to: 13-14
65-65: LGTM! TracingLogger initialization.The
tracingLoggeris correctly initialized usingdistributedTracing.getTracingLogger(traceContext), following the standard pattern for distributed tracing.
166-166: LGTM! All logging calls updated to tracingLogger.All logging calls within
_ensureAvailableUsershave been correctly updated to usetracingLoggerinstead of the previous logger, enabling distributed tracing throughout the availability checking process.Also applies to: 175-175, 205-205, 209-209, 218-218, 224-224, 233-233, 247-247, 252-252
260-260: LGTM! Public signature unchanged.The public
ensureAvailableUsersexport correctly wraps the internal_ensureAvailableUserswithwithReporting, maintaining backward compatibility while enabling distributed tracing internally.packages/features/bookings/lib/handleNewBooking.ts (11)
27-29: LGTM! Import changes for distributed tracing.The added imports for
TraceContextanddistributedTracingcorrectly support the distributed tracing integration. Import reordering has no functional impact.Also applies to: 69-70
405-405: LGTM! Optional traceContext parameter added.The optional
traceContextparameter inBookingHandlerInputenables distributed tracing while maintaining backward compatibility with existing callers.
443-443: LGTM! Trace context creation with safe metadata.The trace context is correctly created or updated with non-PII metadata (eventTypeId, userId, eventTypeSlug). The tracingLogger is properly initialized from the traceContext.
Note: A past review comment flagged PII concerns with a
userInfofield in trace meta, but that field is not present in the current code. The current implementation correctly excludes PII from trace metadata.Also applies to: 449-462
464-469: LGTM! Initial booking creation logging.The initial logging statement correctly uses
tracingLogger.infowith appropriate non-PII context data.
537-537: LGTM! TraceId included in HttpError responses.Including
traceIdin error data enables end-to-end tracing and helps with debugging by correlating client-side errors with server-side logs.Also applies to: 547-547, 565-565, 572-572, 717-717, 1712-1712, 1957-1957, 2155-2155
518-518: LGTM! BookingEmailSmsHandler uses tracingLogger.The
BookingEmailSmsHandleris correctly initialized withtracingLogger, enabling traced logging for email/SMS operations.
687-687: LGTM! TraceContext propagated to validation functions.The
traceContextis correctly passed tovalidateBookingTimeIsNotOutOfBoundsandvalidateEventLength, enabling distributed tracing through the validation layer.Also applies to: 695-695
852-852: LGTM! TraceContext propagated to ensureAvailableUsers.All calls to
ensureAvailableUserscorrectly include thetraceContextparameter, enabling distributed tracing throughout user availability verification.Also applies to: 867-867, 886-886, 915-915, 1027-1027
1723-1723: LGTM! TraceContext passed to EventManager.The
EventManageris correctly instantiated withtraceContext, enabling distributed tracing for calendar event creation and management operations.
2197-2197: LGTM! TraceContext propagated to webhook, workflow, and scheduling functions.The
traceContextis correctly passed to all webhook, workflow, and scheduling functions (handleWebhookTrigger,scheduleTrigger,scheduleMandatoryReminder,WorkflowService.scheduleWorkflowsForNewBooking,scheduleNoShowTriggers), enabling end-to-end distributed tracing across the entire booking lifecycle.Also applies to: 2294-2294, 2307-2307, 2334-2334, 2346-2346, 2407-2407, 2422-2422, 2443-2443
652-652: LGTM! Comprehensive tracingLogger usage throughout booking flow.All logging calls throughout the booking handler have been correctly updated to use
tracingLogger, enabling traced logging at all critical points including:
- Event type and availability logging
- User selection and fallback logic
- Error conditions and warnings
- Booking creation and updates
- Integration results (EventManager, webhooks, workflows)
- Payment and scheduling operations
This provides comprehensive observability for the entire booking lifecycle.
Also applies to: 744-744, 894-894, 922-922, 951-951, 1037-1037, 1102-1102, 1198-1198, 1213-1214, 1546-1546, 1707-1707, 1730-1730, 1761-1761, 1832-1832, 1855-1855, 1985-1985, 2063-2063, 2080-2080, 2125-2125, 2225-2225, 2256-2256, 2320-2320, 2356-2356, 2387-2387, 2425-2425, 2447-2447
apps/web/test/lib/confirm.handler.test.ts (2)
15-15: LGTM!The import correctly adds the
distributedTracingutility needed to create trace contexts for testing.
95-97: LGTM!The test correctly adapts to the new
traceContextrequirement. The operation name and metadata are appropriate for test tracing.packages/features/bookings/lib/handleConfirmation.ts (4)
4-5: LGTM!The imports correctly add the necessary distributed tracing types and utilities.
Also applies to: 22-23
88-104: LGTM!The trace context setup correctly creates a child span with appropriate metadata and obtains a tracing logger. The metadata comprehensively captures the booking context for observability.
121-121: LGTM!All logger calls have been correctly replaced with
tracingLogger, ensuring proper trace context propagation in error logs. The error messages include comprehensive context for debugging.Also applies to: 158-158, 394-394, 506-512, 569-574, 607-607, 612-612
377-377: LGTM!The
spanContextis correctly propagated to downstream async operations, enabling end-to-end trace continuity across the booking confirmation workflow.Also applies to: 389-389, 475-475
...ges/features/bookings/lib/handleSeats/reschedule/attendee/attendeeRescheduleSeatedBooking.ts
Outdated
Show resolved
Hide resolved
packages/features/bookings/lib/handleSeats/reschedule/owner/combineTwoSeatedBookings.ts
Outdated
Show resolved
Hide resolved
packages/features/bookings/lib/handleSeats/reschedule/owner/moveSeatedBookingToNewTimeSlot.ts
Outdated
Show resolved
Hide resolved
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
11 issues found across 47 files
Prompt for AI agents (all 11 issues)
Understand the root cause of the following 11 issues and fix them.
<file name="packages/features/ee/workflows/lib/reminders/scheduleMandatoryReminder.ts">
<violation number="1" location="packages/features/ee/workflows/lib/reminders/scheduleMandatoryReminder.ts:42">
Rule violated: **Avoid Logging Sensitive Information**
`seatReferenceUid` is being logged through the tracing meta prefixes, exposing a sensitive seat identifier. Please omit or fully redact this value before attaching it to the tracing context to comply with the "Avoid Logging Sensitive Information" guideline.</violation>
</file>
<file name="packages/features/bookings/lib/handleSeats/handleSeats.ts">
<violation number="1" location="packages/features/bookings/lib/handleSeats/handleSeats.ts:64">
Rule violated: **Avoid Logging Sensitive Information**
Please avoid logging raw bookerEmail values. Email addresses are PII and must not be written to logs per the "Avoid Logging Sensitive Information" guideline.</violation>
</file>
<file name="packages/trpc/server/createContext.ts">
<violation number="1" location="packages/trpc/server/createContext.ts:70">
Rule violated: **Avoid Logging Sensitive Information**
Storing the session `userId` in the distributed trace metadata causes every log generated via this trace logger to emit `userId:<value>` as part of the log prefix, exposing PII and violating the “Avoid Logging Sensitive Information” guideline.</violation>
</file>
<file name="apps/web/pages/api/book/event.ts">
<violation number="1" location="apps/web/pages/api/book/event.ts:71">
Rule violated: **Avoid Logging Sensitive Information**
Do not log `booking.uid`; it exposes the unique booking identifier and leaks sensitive booking data, violating the "Avoid Logging Sensitive Information" rule.</violation>
</file>
<file name="packages/features/bookings/lib/handleSeats/reschedule/owner/moveSeatedBookingToNewTimeSlot.ts">
<violation number="1" location="packages/features/bookings/lib/handleSeats/reschedule/owner/moveSeatedBookingToNewTimeSlot.ts:81">
Rule violated: **Avoid Logging Sensitive Information**
`tracingLogger.error` now logs the organizer's name, which is personally identifiable information and violates our prohibition on logging sensitive data.</violation>
</file>
<file name="packages/features/webhooks/lib/scheduleTrigger.ts">
<violation number="1" location="packages/features/webhooks/lib/scheduleTrigger.ts:309">
Rule violated: **Avoid Logging Sensitive Information**
Logging the raw subscriberUrl exposes potentially sensitive webhook endpoints (e.g., tokens in the URL), which violates our "Avoid Logging Sensitive Information" rule. Please redact or omit this value before logging.</violation>
</file>
<file name="packages/features/bookings/lib/service/RegularBookingService.ts">
<violation number="1" location="packages/features/bookings/lib/service/RegularBookingService.ts:1282">
Rule violated: **Avoid Logging Sensitive Information**
Please avoid logging guest email addresses; this tracingLogger.info call emits raw PII and violates the Avoid Logging Sensitive Information rule for bookings.</violation>
</file>
<file name="packages/features/bookings/lib/handleNewBooking/validateEventLength.ts">
<violation number="1" location="packages/features/bookings/lib/handleNewBooking/validateEventLength.ts:27">
Rule violated: **Avoid Logging Sensitive Information**
Adding reqBodyStart/reqBodyEnd to the tracing metadata stores users’ scheduling timestamps in our logs, which violates the “Avoid Logging Sensitive Information” rule. Please remove these fields or replace them with non-sensitive aggregates before emitting the span.</violation>
</file>
<file name="packages/features/webhooks/lib/handleWebhookScheduledTriggers.ts">
<violation number="1" location="packages/features/webhooks/lib/handleWebhookScheduledTriggers.ts:89">
Rule violated: **Avoid Logging Sensitive Information**
The new tracing log writes the full webhook subscriberUrl, which commonly contains embedded secret tokens. Exposing that URL in logs leaks credentials and breaks the "Avoid Logging Sensitive Information" rule.</violation>
</file>
<file name="packages/lib/server/defaultResponder.ts">
<violation number="1" location="packages/lib/server/defaultResponder.ts:24">
Using optional chaining only on the first replace means that when endpointRoute is undefined (as in many defaultResponder usages), this line tries to call .replace on undefined and the request crashes. Please guard the second replace as well.</violation>
</file>
<file name="packages/features/bookings/lib/EventManager.ts">
<violation number="1" location="packages/features/bookings/lib/EventManager.ts:299">
Logging the raw `event.title` exposes PII in traces. Please sanitize the title (e.g., via `getPiiFreeCalendarEvent(event).title`) before writing it to the log.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| organizerId: evt.organizer.id?.toString() || "unknown", | ||
| requiresConfirmation: String(requiresConfirmation), | ||
| hideBranding: String(hideBranding), | ||
| seatReferenceUid: seatReferenceUid || "null", |
There was a problem hiding this comment.
Rule violated: Avoid Logging Sensitive Information
seatReferenceUid is being logged through the tracing meta prefixes, exposing a sensitive seat identifier. Please omit or fully redact this value before attaching it to the tracing context to comply with the "Avoid Logging Sensitive Information" guideline.
Prompt for AI agents
Address the following comment on packages/features/ee/workflows/lib/reminders/scheduleMandatoryReminder.ts at line 42:
<comment>`seatReferenceUid` is being logged through the tracing meta prefixes, exposing a sensitive seat identifier. Please omit or fully redact this value before attaching it to the tracing context to comply with the "Avoid Logging Sensitive Information" guideline.</comment>
<file context>
@@ -28,9 +28,34 @@ async function _scheduleMandatoryReminder({
+ organizerId: evt.organizer.id?.toString() || "unknown",
+ requiresConfirmation: String(requiresConfirmation),
+ hideBranding: String(hideBranding),
+ seatReferenceUid: seatReferenceUid || "null",
+ isPlatformNoEmail: String(isPlatformNoEmail),
+ };
</file context>
| seatReferenceUid: seatReferenceUid || "null", | |
| seatReferenceUid: seatReferenceUid ? "[redacted]" : "null", |
| eventTypeSlug: eventType.slug, | ||
| rescheduleUid, | ||
| reqBookingUid, | ||
| bookerEmail, |
There was a problem hiding this comment.
Rule violated: Avoid Logging Sensitive Information
Please avoid logging raw bookerEmail values. Email addresses are PII and must not be written to logs per the "Avoid Logging Sensitive Information" guideline.
Prompt for AI agents
Address the following comment on packages/features/bookings/lib/handleSeats/handleSeats.ts at line 64:
<comment>Please avoid logging raw bookerEmail values. Email addresses are PII and must not be written to logs per the "Avoid Logging Sensitive Information" guideline.</comment>
<file context>
@@ -34,10 +36,36 @@ const handleSeats = async (newSeatedBookingObject: NewSeatedBookingObject) => {
+ eventTypeSlug: eventType.slug,
+ rescheduleUid,
+ reqBookingUid,
+ bookerEmail,
+ hasOriginalRescheduledBooking: !!originalRescheduledBooking,
+ isReschedule: !!rescheduleUid,
</file context>
| bookerEmail, | |
| hasBookerEmail: Boolean(bookerEmail), |
| export async function createContextInner(opts: CreateInnerContextOptions): Promise<InnerContext> { | ||
| const traceContext = distributedTracing.createTrace("trpc_request", { | ||
| meta: { | ||
| userId: opts.session?.user?.id?.toString() || "anonymous", |
There was a problem hiding this comment.
Rule violated: Avoid Logging Sensitive Information
Storing the session userId in the distributed trace metadata causes every log generated via this trace logger to emit userId:<value> as part of the log prefix, exposing PII and violating the “Avoid Logging Sensitive Information” guideline.
Prompt for AI agents
Address the following comment on packages/trpc/server/createContext.ts at line 70:
<comment>Storing the session `userId` in the distributed trace metadata causes every log generated via this trace logger to emit `userId:<value>` as part of the log prefix, exposing PII and violating the “Avoid Logging Sensitive Information” guideline.</comment>
<file context>
@@ -62,10 +65,17 @@ export type InnerContext = CreateInnerContextOptions & {
export async function createContextInner(opts: CreateInnerContextOptions): Promise<InnerContext> {
+ const traceContext = distributedTracing.createTrace("trpc_request", {
+ meta: {
+ userId: opts.session?.user?.id?.toString() || "anonymous",
+ },
+ });
</file context>
| }); | ||
|
|
||
| tracingLogger.info("API book event request completed successfully", { | ||
| bookingUid: booking?.uid, |
There was a problem hiding this comment.
Rule violated: Avoid Logging Sensitive Information
Do not log booking.uid; it exposes the unique booking identifier and leaks sensitive booking data, violating the "Avoid Logging Sensitive Information" rule.
Prompt for AI agents
Address the following comment on apps/web/pages/api/book/event.ts at line 71:
<comment>Do not log `booking.uid`; it exposes the unique booking identifier and leaks sensitive booking data, violating the "Avoid Logging Sensitive Information" rule.</comment>
<file context>
@@ -52,8 +63,13 @@ async function handler(req: NextApiRequest & { userId?: number }) {
});
+
+ tracingLogger.info("API book event request completed successfully", {
+ bookingUid: booking?.uid,
+ });
// const booking = await createBookingThroughFactory();
</file context>
| bookingUid: booking?.uid, | |
| // booking UID intentionally not logged to avoid leaking sensitive data |
| message: "Booking Rescheduling failed", | ||
| }; | ||
| loggerWithEventDetails.error(`Booking ${organizerUser.name} failed`, JSON.stringify({ error, results })); | ||
| tracingLogger.error(`Booking ${organizerUser.name} failed`, JSON.stringify({ error, results })); |
There was a problem hiding this comment.
Rule violated: Avoid Logging Sensitive Information
tracingLogger.error now logs the organizer's name, which is personally identifiable information and violates our prohibition on logging sensitive data.
Prompt for AI agents
Address the following comment on packages/features/bookings/lib/handleSeats/reschedule/owner/moveSeatedBookingToNewTimeSlot.ts at line 81:
<comment>`tracingLogger.error` now logs the organizer's name, which is personally identifiable information and violates our prohibition on logging sensitive data.</comment>
<file context>
@@ -70,7 +78,7 @@ const moveSeatedBookingToNewTimeSlot = async (
message: "Booking Rescheduling failed",
};
- loggerWithEventDetails.error(`Booking ${organizerUser.name} failed`, JSON.stringify({ error, results }));
+ tracingLogger.error(`Booking ${organizerUser.name} failed`, JSON.stringify({ error, results }));
} else {
const metadata: AdditionalInformation = {};
</file context>
| tracingLogger.error(`Booking ${organizerUser.name} failed`, JSON.stringify({ error, results })); | |
| tracingLogger.error("Booking rescheduling failed", JSON.stringify({ error, results })); |
|
|
||
| if (guestsRemoved.length > 0) { | ||
| log.info("Removed guests from the booking", guestsRemoved); | ||
| tracingLogger.info("Removed guests from the booking", guestsRemoved); |
There was a problem hiding this comment.
Rule violated: Avoid Logging Sensitive Information
Please avoid logging guest email addresses; this tracingLogger.info call emits raw PII and violates the Avoid Logging Sensitive Information rule for bookings.
Prompt for AI agents
Address the following comment on packages/features/bookings/lib/service/RegularBookingService.ts at line 1282:
<comment>Please avoid logging guest email addresses; this tracingLogger.info call emits raw PII and violates the Avoid Logging Sensitive Information rule for bookings.</comment>
<file context>
@@ -1246,7 +1279,7 @@ async function handler(
if (guestsRemoved.length > 0) {
- log.info("Removed guests from the booking", guestsRemoved);
+ tracingLogger.info("Removed guests from the booking", guestsRemoved);
}
</file context>
| tracingLogger.info("Removed guests from the booking", guestsRemoved); | |
| tracingLogger.info("Removed guests from the booking", { count: guestsRemoved.length }); |
| }: Props) => { | ||
| const spanContext = traceContext | ||
| ? distributedTracing.createSpan(traceContext, "validate_event_length", { | ||
| reqBodyStart, |
There was a problem hiding this comment.
Rule violated: Avoid Logging Sensitive Information
Adding reqBodyStart/reqBodyEnd to the tracing metadata stores users’ scheduling timestamps in our logs, which violates the “Avoid Logging Sensitive Information” rule. Please remove these fields or replace them with non-sensitive aggregates before emitting the span.
Prompt for AI agents
Address the following comment on packages/features/bookings/lib/handleNewBooking/validateEventLength.ts at line 27:
<comment>Adding reqBodyStart/reqBodyEnd to the tracing metadata stores users’ scheduling timestamps in our logs, which violates the “Avoid Logging Sensitive Information” rule. Please remove these fields or replace them with non-sensitive aggregates before emitting the span.</comment>
<file context>
@@ -18,8 +20,25 @@ const _validateEventLength = ({
}: Props) => {
+ const spanContext = traceContext
+ ? distributedTracing.createSpan(traceContext, "validate_event_length", {
+ reqBodyStart,
+ reqBodyEnd,
+ eventTypeMultipleDuration: JSON.stringify(eventTypeMultipleDuration),
</file context>
| tracingLogger.info("Executing scheduled webhook", { | ||
| bookingId: parsedJobPayload.id, | ||
| triggerEvent: parsedJobPayload.triggerEvent, | ||
| subscriberUrl: job.subscriberUrl, |
There was a problem hiding this comment.
Rule violated: Avoid Logging Sensitive Information
The new tracing log writes the full webhook subscriberUrl, which commonly contains embedded secret tokens. Exposing that URL in logs leaks credentials and breaks the "Avoid Logging Sensitive Information" rule.
Prompt for AI agents
Address the following comment on packages/features/webhooks/lib/handleWebhookScheduledTriggers.ts at line 89:
<comment>The new tracing log writes the full webhook subscriberUrl, which commonly contains embedded secret tokens. Exposing that URL in logs leaks credentials and breaks the "Avoid Logging Sensitive Information" rule.</comment>
<file context>
@@ -73,8 +75,24 @@ export async function handleWebhookScheduledTriggers(prisma: PrismaClient) {
+ tracingLogger.info("Executing scheduled webhook", {
+ bookingId: parsedJobPayload.id,
+ triggerEvent: parsedJobPayload.triggerEvent,
+ subscriberUrl: job.subscriberUrl,
+ originalTraceId: parsedJobPayload._traceContext?.traceId,
+ timeSinceScheduled: parsedJobPayload?.endTime
</file context>
| subscriberUrl: job.subscriberUrl, | |
| subscriberUrl: "[REDACTED]", |
| const tracingLogger = spanContext ? distributedTracing.getTracingLogger(spanContext) : log; | ||
|
|
||
| tracingLogger.info("EventManager.create started", { | ||
| eventTitle: event.title, |
There was a problem hiding this comment.
Logging the raw event.title exposes PII in traces. Please sanitize the title (e.g., via getPiiFreeCalendarEvent(event).title) before writing it to the log.
Prompt for AI agents
Address the following comment on packages/features/bookings/lib/EventManager.ts at line 299:
<comment>Logging the raw `event.title` exposes PII in traces. Please sanitize the title (e.g., via `getPiiFreeCalendarEvent(event).title`) before writing it to the log.</comment>
<file context>
@@ -282,6 +290,16 @@ export default class EventManager {
+ const tracingLogger = spanContext ? distributedTracing.getTracingLogger(spanContext) : log;
+
+ tracingLogger.info("EventManager.create started", {
+ eventTitle: event.title,
+ attendeeCount: event.attendees.length,
+ integrationCount: this.calendarCredentials.length,
</file context>
| eventTitle: event.title, | |
| eventTitle: getPiiFreeCalendarEvent(event).title, |
| traceContext: TraceContext; | ||
| } | ||
|
|
||
| type Handle<T> = (req: TracedRequest, res: NextApiResponse) => Promise<T>; |
There was a problem hiding this comment.
nit: We could introduced TracedResponse as well. For now, it would be just an alias of NextApiResponse
| import { CreationSource } from "@calcom/prisma/enums"; | ||
|
|
||
| async function handler(req: NextApiRequest & { userId?: number }) { | ||
| async function handler(req: NextApiRequest & { userId?: number; traceContext: TraceContext }) { |
There was a problem hiding this comment.
| async function handler(req: NextApiRequest & { userId?: number; traceContext: TraceContext }) { | |
| async function handler(req: TracedRequest & { userId?: number}) { |
| }, | ||
| }); | ||
|
|
||
| tracingLogger.info("API book event request completed successfully", { |
There was a problem hiding this comment.
Do we need these info logs. This could be a sudden increase in book events call and we might not need that?
| const paymentMeta = { | ||
| paymentId: paymentId.toString(), | ||
| bookingId: bookingId.toString(), | ||
| }; |
There was a problem hiding this comment.
I see this requirement of converting every prop of meta to string.
Why don't we keep the type of meta inside DistributedTracing to be number|string and handle it there.
| const spanContext = distributedTracing.createSpan(traceContext, "payment_success_processing", paymentMeta); | ||
| const tracingLogger = distributedTracing.getTracingLogger(spanContext); |
There was a problem hiding this comment.
nit:
| const spanContext = distributedTracing.createSpan(traceContext, "payment_success_processing", paymentMeta); | |
| const tracingLogger = distributedTracing.getTracingLogger(spanContext); | |
| const {tracingLogger, traceSpan} = distributedTracing.createTracingLogger({traceContext, operation, meta: { paymentId: paymentId.toString(), bookingId} ); |
If we have an API like this we could reduce the noise in the code a bit.
hariombalhara
left a comment
hariombalhara
left a comment
There was a problem hiding this comment.
As discussed we are breaking it down into small reviewable PRs
Udit-takkar
changed the title
|
Sub PRs are merged |
5 participants
What does this PR do?
Part of #22969
Follow up:-
distributedTracerReopened PR #22186
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?