Skip to content

feat: implement APM error capturing in login process.#6

Merged
kipsang01 merged 1 commit intomainfrom
staging
Jan 8, 2026
Merged

feat: implement APM error capturing in login process.#6
kipsang01 merged 1 commit intomainfrom
staging

Conversation

@kipsang01
Copy link
Collaborator

@kipsang01 kipsang01 commented Jan 8, 2026

User description

What kind of change does this PR introduce?

eg: Bug fix, feature, docs update, ...

Why was this change needed?

Please link to related issues when possible, and explain WHY you changed things, not WHAT you changed.

Other information:

eg: Did you discuss this change with anybody before working on it (not required, but can be a good idea for bigger changes). Any plans for the future, etc?

Checklist:

Put a "X" in the boxes below to indicate you have followed the checklist;

  • I have read the CONTRIBUTING guide.
  • I checked that there were not similar issues or PRs already open for this.
  • This PR fixes just ONE issue (do not include multiple issues or types of change in the same PR) For example, don't try and fix a UI issue and include new dependencies in the same PR.

PR Type

Enhancement


Description

  • Add APM error capturing to login process

  • Capture provider, redirect, IP, user agent context

  • Gracefully handle APM failures without affecting flow


Diagram Walkthrough

flowchart LR
  A["Login Error Caught"] --> B["Capture Error via APM"]
  B --> C["Include Context Data"]
  C --> D["Handle APM Failures"]
  D --> E["Continue Redirect Flow"]
Loading

File Walkthrough

Relevant files
Enhancement
auth.controller.ts
Add APM error capturing with context metadata                       

apps/backend/src/api/routes/auth.controller.ts

  • Wrap error handling in try-catch block to capture errors via APM
  • Include contextual metadata: provider, redirect URL, IP, user agent,
    token status
  • Gracefully handle APM module failures to preserve existing redirect
    behavior
  • Use optional chaining to safely call APM captureError method
+18/-0   

@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Sensitive data logging

Description: The new APM error reporting sends potentially sensitive user data (e.g., ip, userAgent,
redirect, and provider) as custom metadata, which can lead to privacy/PII exposure in
third-party observability systems and logs if not explicitly minimized, redacted, or
covered by retention/access controls.
auth.controller.ts [389-402]

Referred Code
try {
  // eslint-disable-next-line @typescript-eslint/no-var-requires
  const apm = require('../../apm.init');
  apm?.captureError?.(e instanceof Error ? e : new Error(String(e)), {
    handled: true,
    custom: {
      provider,
      providerUpper: typeof provider === 'string' ? provider.toUpperCase() : provider,
      redirect,
      ip,
      userAgent,
      hasToken: !!token,
    },
  });
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
PII in APM: The new APM error capture attaches ip and userAgent (and potentially redirect) as custom
metadata, which can constitute sensitive/PII telemetry and violates secure logging
constraints unless explicitly approved/redacted.

Referred Code
apm?.captureError?.(e instanceof Error ? e : new Error(String(e)), {
  handled: true,
  custom: {
    provider,
    providerUpper: typeof provider === 'string' ? provider.toUpperCase() : provider,
    redirect,
    ip,
    userAgent,
    hasToken: !!token,
  },

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing audit context: The newly added APM capture records an error but does not clearly provide an auditable
login-event record with a user identifier and explicit outcome, so it is unclear if login
failures are fully traceable per audit requirements.

Referred Code
try {
  // eslint-disable-next-line @typescript-eslint/no-var-requires
  const apm = require('../../apm.init');
  apm?.captureError?.(e instanceof Error ? e : new Error(String(e)), {
    handled: true,
    custom: {
      provider,
      providerUpper: typeof provider === 'string' ? provider.toUpperCase() : provider,
      redirect,
      ip,
      userAgent,
      hasToken: !!token,
    },
  });

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Swallowed APM errors: The added nested try/catch intentionally ignores APM/reporting failures without any
fallback logging, which may reduce diagnosability depending on operational requirements.

Referred Code
try {
  // eslint-disable-next-line @typescript-eslint/no-var-requires
  const apm = require('../../apm.init');
  apm?.captureError?.(e instanceof Error ? e : new Error(String(e)), {
    handled: true,
    custom: {
      provider,
      providerUpper: typeof provider === 'string' ? provider.toUpperCase() : provider,
      redirect,
      ip,
      userAgent,
      hasToken: !!token,
    },
  });
} catch {
  // Ignore APM/reporting failures; preserve redirect behavior.
}

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Preload APM module once

Move the dynamic require('../../apm.init') call out of the catch block and
preload it at the module scope to prevent repeated, inefficient filesystem
lookups on each error.

apps/backend/src/api/routes/auth.controller.ts [389-405]

 try {
-  // eslint-disable-next-line @typescript-eslint/no-var-requires
-  const apm = require('../../apm.init');
   apm?.captureError?.(e instanceof Error ? e : new Error(String(e)), {
     handled: true,
     custom: {
       provider,
       providerUpper: typeof provider === 'string' ? provider.toUpperCase() : provider,
       redirect,
       ip,
       userAgent,
       hasToken: !!token,
     },
   });
 } catch {
   // Ignore APM/reporting failures; preserve redirect behavior.
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: This is a valid suggestion for performance and code structure. Requiring the module on every error is inefficient, and loading it once at the module or class level is a better practice.

Low
Remove redundant type check

Remove the redundant typeof provider === 'string' check, as provider is already
typed as a string, and directly use provider.toUpperCase().

apps/backend/src/api/routes/auth.controller.ts [396]

-providerUpper: typeof provider === 'string' ? provider.toUpperCase() : provider,
+providerUpper: provider.toUpperCase(),
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies a redundant type check for the provider variable, which is already typed as a string. Removing it simplifies the code and improves readability.

Low
  • More

@kipsang01 kipsang01 merged commit 7964ed3 into main Jan 8, 2026
0 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant