Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Don't log 4xx as error #14649

Merged
merged 6 commits into from
May 22, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ export const ApiConfig = {
fetchApi: createEnhancedFetch({
name: 'clients-administration-of-occupational-safety-and-health',
logErrorResponseBody: true,
treat400ResponsesAsErrors: true,
organizationSlug: 'vinnueftirlitid',
}),
basePath: 'https://ws.ver.is/namskeid',
Expand Down
1 change: 0 additions & 1 deletion libs/clients/aircraft-registry/src/lib/apiConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ export const ApiConfig = {
fetchApi: createEnhancedFetch({
name: 'clients-aircraft-registry',
organizationSlug: 'samgongustofa',
treat400ResponsesAsErrors: true,
}),
basePath: `${xroadConfig.xRoadBasePath}/r1/${config.xRoadServicePath}`,
headers: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ export const ApiConfig = {
fetchApi: createEnhancedFetch({
name: 'clients-housing-benefit-calculator',
logErrorResponseBody: true,
treat400ResponsesAsErrors: true,
RunarVestmann marked this conversation as resolved.
Show resolved Hide resolved
}),
basePath: `${xroadConfig.xRoadBasePath}/r1/${config.xRoadServicePath}`,
headers: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ export const ApiConfig = {
name: 'clients-icelandic-government-institution-vacancies',
organizationSlug: 'fjarsysla-rikisins',
logErrorResponseBody: true,
treat400ResponsesAsErrors: true,
timeout: 20000,
}),
basePath: `${xroadConfig.xRoadBasePath}/r1/${config.xRoadServicePath}`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ export class HealthInsuranceV2Client {
fetchApi: createEnhancedFetch({
name: 'clients-health-insurance',
organizationSlug: 'sjukratryggingar',
treat400ResponsesAsErrors: true,
saevarma marked this conversation as resolved.
Show resolved Hide resolved
logErrorResponseBody: true,
timeout: 20000, // needed because the external service is taking a while to respond to submitting the document
}),
Expand Down
1 change: 0 additions & 1 deletion libs/clients/middlewares/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ A new library providing an createEnhancedFetch function.
- `name: string` - Name of fetch function. Used in logs and opossum stats.
- `enableCircuitBreaker?: boolean` - Should use circuit breaker for requests. Defaults to `true`.
- `timeout?: number | false` - Timeout for requests. Logged and thrown as errors. May cause circuit breaker to open. Defaults to `10000`ms. Can be disabled by passing false.
saevarma marked this conversation as resolved.
Show resolved Hide resolved
- `treat400ResponsesAsErrors?: boolean` - If `true`, then too many 400 responses may cause the circuit to open. Either way these responses will be logged and thrown. Defaults to `false`.
- `logErrorResponseBody?: boolean` - If `true`, then non-200 response bodies will be consumed and included in the error object and logged as `body`.
- `keepAlive?: boolean | number` - Configures keepAlive for requests. If `false`, never reuse connections. If `true`, reuse connection with a maximum idle timeout of 10 seconds. By passing a number you can override the idle connection timeout. Defaults to `true`.
- `clientCertificate?: ClientCertificateOptions` - Configures client certificate for requests.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ describe('EnhancedFetch', () => {

it('can be configured to open circuit for 400 errors', async () => {
// Arrange
env = setupTestEnv({ treat400ResponsesAsErrors: true })
saevarma marked this conversation as resolved.
Show resolved Hide resolved
env = setupTestEnv({})
env.fetch.mockResolvedValue(fakeResponse('Error', { status: 400 }))
await env.enhancedFetch(testUrl).catch(() => null)

Expand Down
9 changes: 0 additions & 9 deletions libs/clients/middlewares/src/lib/createEnhancedFetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,6 @@ export interface EnhancedFetchOptions {
*/
forwardAuthUserAgent?: boolean

/**
* By default, 400 responses are considered warnings and will not open the circuit.
* Either way they will be logged and thrown.
*/
treat400ResponsesAsErrors?: boolean

/**
* If true (default), Enhanced Fetch will log error response bodies.
* Should be set to false if error objects may have sensitive information or PII.
Expand Down Expand Up @@ -159,7 +153,6 @@ export const createEnhancedFetch = (
metricsClient = new DogStatsD({ prefix: `${options.name}.` }),
organizationSlug,
} = options
const treat400ResponsesAsErrors = options.treat400ResponsesAsErrors === true
const freeSocketTimeout =
typeof keepAlive === 'number'
? keepAlive
Expand Down Expand Up @@ -207,7 +200,6 @@ export const createEnhancedFetch = (
builder.wrap(withCircuitBreaker, {
name,
logger,
treat400ResponsesAsErrors,
opossum,
})
}
Expand All @@ -233,7 +225,6 @@ export const createEnhancedFetch = (
builder.wrap(withErrorLog, {
name,
logger,
treat400ResponsesAsErrors,
})

return builder.getFetch()
Expand Down
16 changes: 6 additions & 10 deletions libs/clients/middlewares/src/lib/withCircuitBreaker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,28 +5,24 @@ import { MiddlewareAPI } from './nodeFetch'
import { FetchError } from './FetchError'

export interface CircuitBreakerOptions {
treat400ResponsesAsErrors: boolean
opossum: CircuitBreaker.Options
fetch: MiddlewareAPI
name: string
logger: Logger
}

export function withCircuitBreaker({
treat400ResponsesAsErrors,
opossum,
fetch,
name,
logger,
}: CircuitBreakerOptions): MiddlewareAPI {
const errorFilter = treat400ResponsesAsErrors
? opossum?.errorFilter
: (error: FetchError) => {
if (error.name === 'FetchError' && error.status < 500) {
return true
}
return opossum?.errorFilter?.(error) ?? false
}
const errorFilter = (error: FetchError) => {
if (error.name === 'FetchError' && error.status < 500) {
return true
}
return opossum?.errorFilter?.(error) ?? false
}

const breaker = new CircuitBreaker(fetch, {
name,
Expand Down
8 changes: 1 addition & 7 deletions libs/clients/middlewares/src/lib/withErrorLog.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,23 +6,17 @@ import { FetchError } from './FetchError'
interface ErrorLogOptions extends FetchMiddlewareOptions {
name: string
logger: Logger
treat400ResponsesAsErrors: boolean
}

export function withErrorLog({
name,
fetch,
treat400ResponsesAsErrors,
logger,
}: ErrorLogOptions): MiddlewareAPI {
return (request) => {
return fetch(request).catch((error: Error) => {
const logLevel =
error instanceof FetchError &&
error.status < 500 &&
!treat400ResponsesAsErrors
? 'warn'
: 'error'
error instanceof FetchError && error.status < 500 ? 'warn' : 'error'
const cacheStatus =
(error instanceof FetchError &&
error.response.headers.get('cache-status')) ??
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ export const ApiConfig = {
fetchApi: createEnhancedFetch({
name: 'clients-ship-registry',
logErrorResponseBody: true,
treat400ResponsesAsErrors: true,
}),
basePath: `${xroadConfig.xRoadBasePath}/r1/${config.xRoadServicePath}`,
headers: {
Expand Down
Loading