Skip to content

Tag upstream API errors at error level with service/upstreamStatus/at… #848

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.

Sign up for GitHub

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

Open
swdpcomputing wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Tag upstream API errors at error level with service/upstreamStatus/at… #848

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d83cbce
Tag upstream API errors at error level with service/upstreamStatus/at…
swdpcomputing Apr 29, 2026
28aeb87
abstract log-codes as module over 500 limit
swdpcomputing Apr 29, 2026
28a12bc
fix sonar issues
swdpcomputing Apr 29, 2026
359288b
Merge branch 'main' into bugfix/TGC-1057-logging-be-502-to-fe-500
DavidBarker-EE Apr 30, 2026
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
6 changes: 3 additions & 3 deletions src/__mocks__/logger-mocks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { vi } from 'vitest'
import { LogCodes } from '~/src/server/common/helpers/logging/log-codes-definition.js'
import { LogCodes } from '~/src/server/common/helpers/logging/log-codes/definition.js'

export const mockLoggerFactory = () => ({
info: vi.fn(),
Expand All @@ -15,14 +15,14 @@ export const mockLoggerFactoryWithCustomMethods = (customMethods = {}) => ({

/**
* @typedef {Object} MockLogCodesDefinition
* @property {import('~/src/server/common/helpers/logging/log-codes-definition.js').LogTypes.LogLevel} level
* @property {import('~/src/server/common/helpers/logging/log-codes/definition.js').LogTypes.LogLevel} level
* @property {import('vitest').Mock} messageFunc
*/

/**
* Auto-generate mock LogCodes from real LogCodes
* Each messageFunc becomes a vi.fn() for test assertions
* @param {typeof import('~/src/server/common/helpers/logging/log-codes-definition.js').LogCodes} codes
* @param {typeof import('~/src/server/common/helpers/logging/log-codes/definition.js').LogCodes} codes
* @returns {Record<string, Record<string, MockLogCodesDefinition>>}
*/
const autoMockLogCodes = (codes) =>
Expand Down
9 changes: 4 additions & 5 deletions src/contracts/grants-ui-gas.contract.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,10 @@ vi.mock('../server/common/helpers/retry.js', () => ({
retry: (operation) => operation()
}))

vi.mock('../server/common/helpers/logging/log.js', () => ({
debug: vi.fn(),
log: vi.fn(),
logger: { debug: vi.fn(), info: vi.fn(), error: vi.fn(), warn: vi.fn() }
}))
vi.mock('../server/common/helpers/logging/log.js', async () => {
const { mockLogHelper } = await import('../__mocks__/logger-mocks.js')
return mockLogHelper()
})

function createProvider() {
return new PactV4({
Expand Down
16 changes: 4 additions & 12 deletions src/contracts/v1/land-grants.client.contract.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,10 @@ import path from 'path'
import { vi } from 'vitest'
import { postToLandGrantsApi } from '~/src/server/land-grants/services/land-grants.client'

vi.mock('~/src/server/common/helpers/logging/log.js', () => ({
LogCodes: {
LAND_GRANTS: {
API_REQUEST: { level: 'info', messageFunc: vi.fn() }
}
},
log: vi.fn(),
debug: vi.fn(),
logger: {
debug: vi.fn()
}
}))
vi.mock('~/src/server/common/helpers/logging/log.js', async () => {
const { mockLogHelper } = await import('~/src/__mocks__/logger-mocks.js')
return mockLogHelper()
})

vi.mock('~/src/server/common/helpers/retry.js', () => ({
retry: (operation) => operation()
Expand Down
16 changes: 4 additions & 12 deletions src/contracts/v2/land-grants.client.contract.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,10 @@ import path from 'path'
import { vi } from 'vitest'
import { postToLandGrantsApi } from '~/src/server/land-grants/services/land-grants.client'

vi.mock('~/src/server/common/helpers/logging/log.js', () => ({
LogCodes: {
LAND_GRANTS: {
API_REQUEST: { level: 'info', messageFunc: vi.fn() }
}
},
log: vi.fn(),
debug: vi.fn(),
logger: {
debug: vi.fn()
}
}))
vi.mock('~/src/server/common/helpers/logging/log.js', async () => {
const { mockLogHelper } = await import('~/src/__mocks__/logger-mocks.js')
return mockLogHelper()
})

vi.mock('~/src/server/common/helpers/retry.js', () => ({
retry: (operation) => operation()
Expand Down
17 changes: 15 additions & 2 deletions src/server/agreements/controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { config } from '~/src/config/config.js'
import { statusCodes } from '~/src/server/common/constants/status-codes.js'
import Jwt from '@hapi/jwt'
import { SystemError } from '~/src/server/common/utils/errors/SystemError.js'
import { log, debug } from '~/src/server/common/helpers/logging/log.js'
import { log } from '~/src/server/common/helpers/logging/log.js'
import { LogCodes } from '~/src/server/common/helpers/logging/log-codes.js'

/**
Expand Down Expand Up @@ -76,6 +76,19 @@ function buildProxyHeaders(token, request) {
}
}

function logAgreementsUpstreamError(request, error) {
log(
LogCodes.SYSTEM.EXTERNAL_API_ERROR,
{
endpoint: 'agreements',
service: 'farming-grants-agreements-ui',
upstreamStatus: error.statusCode ?? error.output?.statusCode ?? error.status ?? null,
errorMessage: error.message
},
request
)
}

/**
* Controller for the agreements API
* @satisfies {Partial<ServerRoute>}
Expand Down Expand Up @@ -108,7 +121,7 @@ export const getAgreementController = {

return apiResponse
} catch (error) {
debug(LogCodes.SYSTEM.EXTERNAL_API_ERROR, { endpoint: 'agreements', errorMessage: error.message }, request)
logAgreementsUpstreamError(request, error)
Copy link
Copy Markdown
Contributor

@DavidBarker-EE DavidBarker-EE Apr 30, 2026

Choose a reason for hiding this comment

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

I'm not sure about this one 😂 clearly trying to get around the ESLint rule by encapsulating the log in another function. Let's discuss this with @alanplatt, as it might be wrong to wholly deny log in catch blocks.

Copy link
Copy Markdown
Contributor Author

@swdpcomputing swdpcomputing May 1, 2026

Choose a reason for hiding this comment

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

Not dodging the rule! log/logger are already on the exclude list in eslint.config.js. Pulled it into a helper because the payload got chunky. Same shape as handleVerificationError in verify-token.js. Happy to inline it, or chat with Alan.

Copy link
Copy Markdown
Contributor

@DavidBarker-EE DavidBarker-EE May 1, 2026

Choose a reason for hiding this comment

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

hehe the function logAgreementsUpstreamError is just a wrapper for log though right? Reason I mentioned Alan here is that he didn't want catch blocks to be using log, maybe that's not the right approach?


if (error.message.includes('Missing required configuration')) {
return h
Expand Down
20 changes: 20 additions & 0 deletions src/server/agreements/controller.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { getAgreementController } from './controller.js'
import { config } from '~/src/config/config.js'
import Jwt from '@hapi/jwt'
import { mockHapiRequest, mockHapiResponseToolkit } from '~/src/__mocks__/hapi-mocks.js'
import { log } from '~/src/server/common/helpers/logging/log.js'

vi.mock('~/src/config/config.js', async () => {
const { mockConfigSimple } = await import('~/src/__mocks__')
Expand Down Expand Up @@ -449,6 +450,25 @@ describe('Agreements Controller', () => {
})
})

test('should log EXTERNAL_API_ERROR with service and upstreamStatus on proxy 5xx', async () => {
const proxyError = new Error('Bad Gateway')
proxyError.output = { statusCode: 502 }
mockH.proxy.mockRejectedValue(proxyError)

await getAgreementController.handler(mockRequest, mockH)

expect(log).toHaveBeenCalledWith(
expect.objectContaining({ level: 'error' }),
expect.objectContaining({
endpoint: 'agreements',
service: 'farming-grants-agreements-ui',
upstreamStatus: 502,
errorMessage: 'Bad Gateway'
}),
mockRequest
)
})

test('should not include error details in production', async () => {
const originalNodeEnv = process.env.NODE_ENV
process.env.NODE_ENV = 'production'
Expand Down
2 changes: 1 addition & 1 deletion src/server/auth/services/whitelist.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ class WhitelistService {
/**
* Helper method to log whitelist events with consistent structure
* @param {object} logCode - Logging options.
* @param {import('../../common/helpers/logging/log-codes-definition.js').LogTypes.LogLevel} logCode.level - The log level.
* @param {import('../../common/helpers/logging/log-codes/definition.js').LogTypes.LogLevel} logCode.level - The log level.
* @param {Function} logCode.messageFunc - A function that creates an interpolated message string
* @param {string} crn - The user's CRN
* @param {string} sbi - The SBI number
Expand Down
Loading
Loading