Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to improve observability and address a dev crash by enhancing HTTP error logging, adjusting Datadog HTTP client propagation behavior, and standardizing outbound Topcoder API base URLs.
Changes:
- Added
logger.logHttpErrorand expanded redaction to include Authorization fields. - Switched several outbound axios calls from
API_BASE_URLto the newTOPCODER_API_URL, and added detailed logging forgetSkillfailures. - Added Datadog HTTP client propagation blocking for the standardized-skills endpoint, and introduced
mise.tomlto pin Node.js tooling.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/common/logger.js | Adds structured HTTP client error logging and expands sensitive-field redaction. |
| src/common/helper.js | Routes outbound Topcoder API calls through TOPCODER_API_URL and logs standardized-skills API failures. |
| config/default.js | Introduces TOPCODER_API_URL configuration for outbound platform API calls. |
| app.js | Configures dd-trace HTTP client propagation blocking for a standardized-skills endpoint. |
| mise.toml | Pins Node.js tool version for local/dev tooling consistency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // The standardized-skills API rejects Datadog propagation headers on this endpoint. | ||
| propagationBlocklist: [/api\.topcoder-dev\.com\/v5\/standardized-skills/] |
There was a problem hiding this comment.
propagationBlocklist is hard-coded to the dev domain (api.topcoder-dev.com). If this service is pointed at a different Topcoder API host (e.g., prod via TOPCODER_API_URL), Datadog propagation headers will still be injected and the crash/400s may persist. Consider matching on the standardized-skills path regardless of host (or deriving the host from configuration and supporting both dev/prod).
| // The standardized-skills API rejects Datadog propagation headers on this endpoint. | |
| propagationBlocklist: [/api\.topcoder-dev\.com\/v5\/standardized-skills/] | |
| // The standardized-skills API rejects Datadog propagation headers on this endpoint, | |
| // regardless of which Topcoder API host is configured. | |
| propagationBlocklist: [/\/v5\/standardized-skills(?:\/|$|\?)/] |
There was a problem hiding this comment.
@jmgasper there is a point here. We need to capture for prod env too...
| // used to properly set the header response to api calls for services behind a load balancer | ||
| API_BASE_URL: process.env.API_BASE_URL || 'http://localhost:3000', | ||
| // used for outbound calls to Topcoder platform APIs | ||
| TOPCODER_API_URL: process.env.TOPCODER_API_URL || 'https://api.topcoder-dev.com', |
There was a problem hiding this comment.
TOPCODER_API_URL defaults to the dev API domain. Since config/ only contains default.js and test.js, a missing TOPCODER_API_URL env var in a prod deployment would silently route outbound calls to dev. Consider defaulting to the production domain and overriding for dev via env/config, or adding an environment-specific config (e.g., config/production.js/config/development.js) to make the intended target explicit.
| TOPCODER_API_URL: process.env.TOPCODER_API_URL || 'https://api.topcoder-dev.com', | |
| TOPCODER_API_URL: process.env.TOPCODER_API_URL || 'https://api.topcoder.com', |
| return JSON.parse(JSON.stringify(obj, (name, value) => { | ||
| // Array of field names that should not be logged | ||
| // add field if necessary (password, tokens etc) | ||
| const removeFields = ['userToken'] | ||
| const removeFields = ['userToken', 'authorization', 'Authorization'] | ||
| if (_.includes(removeFields, name)) { |
There was a problem hiding this comment.
_sanitizeObject returns the original object unchanged when JSON serialization fails. Now that this is used for HTTP error logging, a serialization failure (e.g., circular refs/unserializable values in context or response payload) could cause sensitive fields (like Authorization tokens) to be logged unredacted. Consider making the failure path return a safely-redacted fallback (or performing redaction without relying on JSON.stringify).
| logger.logHttpError(err, { | ||
| operation: 'getSkill', | ||
| service: 'standardized-skills-api', | ||
| skillId, | ||
| skillUrl | ||
| }) |
There was a problem hiding this comment.
getSkill now logs the axios error via logger.logHttpError and then rethrows. Since the Express error handler also calls logger.logFullError for all thrown errors, this request will be logged twice on every failure. Consider marking the error as already-logged (e.g., err.logged = true) or centralizing HTTP client error logging in the global error handler to avoid duplicate/error-level log spam.
| logger.logHttpError(err, { | |
| operation: 'getSkill', | |
| service: 'standardized-skills-api', | |
| skillId, | |
| skillUrl | |
| }) | |
| err.operation = 'getSkill' | |
| err.service = 'standardized-skills-api' | |
| err.skillId = skillId | |
| err.skillUrl = skillUrl |
| // used to properly set the header response to api calls for services behind a load balancer | ||
| API_BASE_URL: process.env.API_BASE_URL || 'http://localhost:3000', | ||
| // used for outbound calls to Topcoder platform APIs | ||
| TOPCODER_API_URL: process.env.TOPCODER_API_URL || 'https://api.topcoder-dev.com', |
There was a problem hiding this comment.
@jmgasper reminder to set this one to ParameterStore in prod & update the ECS task def to pick it up...
No description provided.