Skip to content

Add customUpCodes support for HTTP monitors (Backend)#3690

Open
harsh-aghara wants to merge 7 commits into
bluewave-labs:developfrom
harsh-aghara:feat/accept-status-codes-backend
Open

Add customUpCodes support for HTTP monitors (Backend)#3690
harsh-aghara wants to merge 7 commits into
bluewave-labs:developfrom
harsh-aghara:feat/accept-status-codes-backend

Conversation

@harsh-aghara
Copy link
Copy Markdown
Contributor

@harsh-aghara harsh-aghara commented May 31, 2026

Describe your changes

This PR adds backend support for a customUpCodes field to HTTP monitors. This feature allows users to specify which non-2xx HTTP status codes (e.g., 401 Unauthorized, 301 Moved Permanently) should be treated as a successful "UP" status instead of the default "DOWN".

Key Changes:

  • Schema & Types: Added customUpCodes: number[] to the Mongoose Monitor schema and TypeScript types, defaulting to [].
  • Validation: Updated Zod schemas in monitorValidation.ts to validate the new field on monitor creation and updates.
  • Core Logic: Modified HttpProvider to check customUpCodes during both success paths and error paths (when got throws an HTTPError).
  • Geo Checks: Updated GlobalPingService and geoChecksService to ensure geographically distributed HTTP checks also respect the custom status codes.
  • Testing: Added an exhaustive unit test suite in httpProvider.test.ts to cover error scenarios, success overrides, empty arrays, and matcher precedence.

Write your issue number after "Fixes "

Refs #3657

Please ensure all items are checked off before requesting a review. "Checked off" means you need to add an "x" character between brackets so they turn into checkmarks.

  • (Do not skip this or your PR will be closed) I deployed the application locally.
  • (Do not skip this or your PR will be closed) I have performed a self-reviewing and testing of my code.
  • I have included the issue # in the PR.
  • I have added i18n support to visible strings
  • I have not included any files that are not related to my pull request, including package-lock and package-json if dependencies have not changed
  • I didn't use any hardcoded values (otherwise it will not scale, and will make it difficult to maintain consistency across the application).
  • I made sure font sizes, color choices etc are all referenced from the theme. I don't have any hardcoded dimensions.
  • My PR is granular and targeted to one specific feature.
  • I ran npm run format in server and client directories, which automatically formats your code.
  • I took a screenshot or a video and attached to this PR if there is a UI change.

Adds a customUpCodes field to the monitor schema to allow users to specify which HTTP status codes should be treated as UP instead of DOWN. Includes core logic in HttpProvider and unit tests.

Refs bluewave-labs#3657
Copy link
Copy Markdown
Collaborator

@ajhollid ajhollid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't had a chance to do a full review yet, but off the bat this should use the http codes defined in the http library, these should not be arbitrary or self defined numbers.

The http library already properly defines the codes and should be used for type discrimination.

https://nodejs.org/api/http.html#httpstatus_codes

Addresses reviewer feedback by enforcing strict validation on the  array. Previously, the Zod schema accepted any arbitrary number, which could allow invalid or self-defined HTTP codes (e.g., 5000) to be saved to the database.

Refs bluewave-labs#3657
@harsh-aghara harsh-aghara requested a review from ajhollid June 1, 2026 07:59
@harsh-aghara
Copy link
Copy Markdown
Contributor Author

I haven't had a chance to do a full review yet, but off the bat this should use the http codes defined in the http library, these should not be arbitrary or self defined numbers.

The http library already properly defines the codes and should be used for type discrimination.

https://nodejs.org/api/http.html#httpstatus_codes

@ajhollid I just pushed a commit that updates the Zod schemas to validate customUpCodes directly against http.STATUS_CODES. It now uses the http library for discrimination and will reject any fake or arbitrary numbers (like 9999) from being saved. I also added full unit tests for this behavior.

Copy link
Copy Markdown
Member

@Br0wnHammer Br0wnHammer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, the PR looks solid and clean. I have one concern:
got follows redirects by default, so a 301/302 never shows up as the final status; the redirect target's code does. That means adding 3xx codes to `customUpCodes' won't actually work here (the 301 test only passes because the response is mocked). Should 3xx be supported, or should we drop the 301 example?

Copy link
Copy Markdown
Collaborator

@ajhollid ajhollid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a bit more work, please see my comments in the review.

Thanks!

Comment thread server/src/types/monitor.ts Outdated
uptimePercentage?: number;
notifications: string[];
tags: string[];
customUpCodes: number[];
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's properly define an HttpStatusCode type and a set we can use to verify

export const HttpStatusCodes = Object.keys(http.STATUS_CODES).map(Number);
export const HttpStatusCodeSet = new Set(HttpStatusCodes);
export type HttpStatusCode = number;

and use it here to properly type this:

	customUpCodes: HttpStatusCode[];

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

import { DnsRecordTypes, MonitorMatchMethods, MonitorStatuses, MonitorTypes, PageSpeedStrategies } from "@/types/monitor.js";
import http from "node:http";

const httpStatusCode = z.number().refine((code) => code.toString() in http.STATUS_CODES, { message: "Must be a valid HTTP status code" });
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can now be simplified

const httpStatusCode = z.number().refine((code) => HttpStatusCodeSet.has(code), { message: "Must be a valid HTTP status code" });

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simplified this, thanks for the review!


private handleHttpError<T>(error: unknown, monitor: Monitor): MonitorStatusResponse<T> {
if (error instanceof HTTPError || error instanceof RequestError) {
const isCustomUp = error instanceof HTTPError && (monitor.customUpCodes ?? []).includes(error.response?.statusCode);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very hard to read and has a superfluous instance assertion. Why not do something like the following:

const statusCode = error.response?.statusCode;
const statusUp = statusCode !== undefined && monitor.customUpCodes.includes(statusCode);
...
return {
				monitorId: monitor.id,
				teamId: monitor.teamId,
				type: monitor.type,
				status: statusUp,
  				code: statusCode ?? NETWORK_ERROR,
  				message: error.message,
  				responseTime: error.timings?.phases?.total ?? 0,
  				timings: error.timings,
  				payload: null as T,
			};

Something along those lines is clearer and much easier to read.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.
broke down code into more readable parts.

teamId: monitor.teamId,
type: monitor.type,
status: response.ok && matchResult.ok,
status: ((monitor.customUpCodes ?? []).includes(response.statusCode) || response.ok) && matchResult.ok,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This difficult to read line then simply becomes

status: statusUp && matchResult.ok,

It's also worth noting that this is acutally not needed since got throws on non 2xx responses.

This is harmless though and we can leave it in in case we decide to change the got default behaviour.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

async pollForResults(measurementId: string, timeoutMs: number = MAX_POLL_TIMEOUT_MS): Promise<GeoCheckResult[]> {
async pollForResults(measurementId: string, timeoutMs: number = MAX_POLL_TIMEOUT_MS, customUpCodes: number[] = []): Promise<GeoCheckResult[]> {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct type can be used here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.
Updated globalPingService.ts to use correct type.

@harsh-aghara
Copy link
Copy Markdown
Contributor Author

harsh-aghara commented Jun 3, 2026

Overall, the PR looks solid and clean. I have one concern:
got follows redirects by default, so a 301/302 never shows up as the final status; the redirect target's code does. That means adding 3xx codes to `customUpCodes' won't actually work here (the 301 test only passes because the response is mocked). Should 3xx be supported, or should we drop the 301 example?

@Br0wnHammer Good catch, you are right. Since got follows redirects by default, it will return the final status code instead of the 3xx code in production.

To handle this, I see two options:

  1. Restrict 3xx codes: We can update the Zod validation to reject 3xx codes so users don't get confused (and remove the 301 test example).
  2. Set followRedirect: false: We can configure got to stop following redirects. However, this creates a new issue: if the final destination service is actually down, the monitor will still report as "Up" just because it received the initial 3xx response.

I think Option 1 is the safer choice.
What are your thoughts on this @ajhollid ?

Add HttpStatusCode types and simplify provider logic
@harsh-aghara harsh-aghara requested a review from ajhollid June 3, 2026 18:21
@ajhollid
Copy link
Copy Markdown
Collaborator

ajhollid commented Jun 3, 2026

Overall, the PR looks solid and clean. I have one concern:
got follows redirects by default, so a 301/302 never shows up as the final status; the redirect target's code does. That means adding 3xx codes to `customUpCodes' won't actually work here (the 301 test only passes because the response is mocked). Should 3xx be supported, or should we drop the 301 example?

@Br0wnHammer Good catch, you are right. Since got follows redirects by default, it will return the final status code instead of the 3xx code in production.

To handle this, I see two options:

  1. Restrict 3xx codes: We can update the Zod validation to reject 3xx codes so users don't get confused (and remove the 301 test example).
  2. Set followRedirect: false: We can configure got to stop following redirects. However, this creates a new issue: if the final destination service is actually down, the monitor will still report as "Up" just because it received the initial 3xx response.

I think Option 1 is the safer choice. What are your thoughts on this @ajhollid ?

Thanks for making changes here, this is looking good 🚀

I think the issue around redirects isn't straightforward, especially since I don't know how the geo checks handle redirects.

Additionally, got's responseok value is depends on the config, ie if followRedirect is turned on or off.

What about extracting a utility to deal with status that is independent of got's response.ok, instead just returning a value based purely on status codes?

export const isStatusUp = (statusCode, customUpCodes) => {
   return statusCode >= 200 && statusCode < 300 || customUpCodes.includes(statusCode)
}

something like that, which we can use in both the HttpProvider and globalPing provider to ensure consistent values?

@harsh-aghara
Copy link
Copy Markdown
Contributor Author

@ajhollid I've implemented the isStatusUp utility as requested, so the UP status is now determined independently of got.

Copy link
Copy Markdown
Collaborator

@ajhollid ajhollid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The advanced matcher is ignored in the error path. This is as it should have been before this change, but now that there are codes that could be considered as "up" in the error path, the advanced matcher should be run in those cases.

Comment on lines +34 to +42
const statusCode = error.response?.statusCode;
const statusUp = isStatusUp(statusCode, monitor.customUpCodes);

return {
monitorId: monitor.id,
teamId: monitor.teamId,
type: monitor.type,
status: false,
code: error.response?.statusCode ?? NETWORK_ERROR,
status: statusUp,
code: statusCode ?? NETWORK_ERROR,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an issue here.

If we accept that there are error codes that may actually be considered as "Up", we need to run the advanced matcher here as well.

If a user sets advanced matching and a custom code, their advanced matcher will be ignored.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed this in the latest commit.
It now properly runs the advanced matcher for custom up codes too.
Thank you for the review.

When customUpCodes mark an HTTPError as up, parse the response body, apply advancedMatcher.validate, and set status to statusUp && matchResult.ok. Return parsed payload and extracted values to match the success path.
…us-codes-backend

# Conflicts:
#	server/src/service/infrastructure/network/HttpProvider.ts
@harsh-aghara harsh-aghara requested a review from ajhollid June 4, 2026 19:38
Copy link
Copy Markdown
Collaborator

@ajhollid ajhollid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's too much code duplication in the error branch, it's nearly identical to the success branch. Let's extract it to a helper.

Comment on lines +63 to +100
const { jsonPath } = monitor;
const body = error.response?.body ?? "";
const contentType = error.response?.headers?.["content-type"] || "";
const isJson = contentType.includes("application/json");

if (jsonPath && !isJson) {
return {
monitorId: monitor.id,
teamId: monitor.teamId,
type: monitor.type,
status: false,
code: statusCode ?? NETWORK_ERROR,
message: "Response is not JSON",
responseTime,
timings: error.timings,
payload: body as unknown as T,
};
}

let payload: T;
if (isJson) {
try {
payload = JSON.parse(body) as T;
} catch {
payload = body as unknown as T;
}
} else {
payload = body as unknown as T;
}

const matchResult = this.advancedMatcher.validate<T>(payload, monitor);

return {
monitorId: monitor.id,
teamId: monitor.teamId,
type: monitor.type,
status: false,
code: error.response?.statusCode ?? NETWORK_ERROR,
message: error.message,
responseTime: error.timings?.phases?.firstByte ?? error.timings?.phases?.total ?? 0,
status: statusUp && matchResult.ok,
code: statusCode ?? NETWORK_ERROR,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is nearly identical to the code in the success branch. This should be extracted to a private helper function

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure i can create a private method to obey DRY.

@harsh-aghara harsh-aghara requested a review from ajhollid June 5, 2026 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants