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

Improve instrumentation on Authorization Request jobs #588

Merged
merged 3 commits into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 1 addition & 1 deletion .github/workflows/armory.yml
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ jobs:
- name: Install node.js
uses: actions/setup-node@v4
with:
node-version: '21'
node-version: '20'
cache: 'npm'

- name: Install dependencies
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/packages-publish.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ jobs:
- name: Install node.js
uses: actions/setup-node@v4
with:
node-version: '21'
node-version: '20'
registry-url: https://registry.npmjs.org/
cache: 'npm'

Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/packages.yml
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ jobs:
- name: Install node.js
uses: actions/setup-node@v4
with:
node-version: '21'
node-version: '20'
cache: 'npm'

- name: Install dependencies
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/policy-engine.yml
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ jobs:
- name: Install node.js
uses: actions/setup-node@v4
with:
node-version: '21'
node-version: '20'
cache: 'npm'

- name: Install dependencies
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/vault.yml
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ jobs:
- name: Install node.js
uses: actions/setup-node@v4
with:
node-version: '21'
node-version: '20'
cache: 'npm'

- name: Install dependencies
Expand Down
4 changes: 2 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ TERM_GREEN := \033[0;32m
# === Install ===

install:
npm install
npm install --engine-strict

install/ci:
npm ci
npm ci --engine-strict

# === Setup ===

Expand Down
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ dependency `@narval-xyz/armory-mpc-module`.

1. Create a `.npmrc` file in the root of this project.
1. Get the values from someone who has them.
1. Now `npm install` should work.
1. Now `make install` should work.

## Generating a new project

Expand All @@ -133,7 +133,7 @@ packages to NPM.

1. Run `make packages/release` and follow the prompts to bump the
projects' versions.
1. Run `npm install` to update `package-lock.json`.
1. Run `make install` to update `package-lock.json`.
1. Commit and push the changes to your branch.
1. After your branch is merged, manually trigger the [packages pipeline to
publish](https://github.com/narval-xyz/armory/actions/workflows/packages-publish.yml)
Expand Down
2 changes: 2 additions & 0 deletions apps/armory/src/orchestration/orchestration.constant.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,3 +81,5 @@ export const ACTION_REQUEST = new Map<Action, ActionRequestConfig>([
}
]
])

export const OTEL_ATTR_JOB_ID = 'job.id'
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { LoggerService } from '@narval/nestjs-shared'
import { LoggerService, TraceService } from '@narval/nestjs-shared'
import { AuthorizationRequestProcessingJob } from '@narval/policy-engine-shared'
import { OnQueueActive, OnQueueCompleted, OnQueueFailed, Process, Processor } from '@nestjs/bull'
import { Inject } from '@nestjs/common'
import { SpanStatusCode } from '@opentelemetry/api'
import { Job } from 'bull'
import { v4 as uuid } from 'uuid'
import {
Expand All @@ -13,12 +15,19 @@ import { InvalidAttestationSignatureException } from '../../../policy-engine/cor
import { UnreachableClusterException } from '../../../policy-engine/core/exception/unreachable-cluster.exception'
import { AuthorizationRequestAlreadyProcessingException } from '../../core/exception/authorization-request-already-processing.exception'
import { AuthorizationRequestService } from '../../core/service/authorization-request.service'
import { OTEL_ATTR_JOB_ID } from '../../orchestration.constant'

const OTEL_ATTR_JOB_ATTEMPS_MADE = 'job.attempts_made'
const OTEL_ATTR_JOB_FAILED = 'job.failed'
const OTEL_ATTR_JOB_MAX_RETRIES = 'job.max_retries'
const OTEL_ATTR_JOB_RETRIED = 'job.retried'

@Processor(AUTHORIZATION_REQUEST_PROCESSING_QUEUE)
export class AuthorizationRequestProcessingConsumer {
constructor(
private authzService: AuthorizationRequestService,
private logger: LoggerService
private logger: LoggerService,
@Inject(TraceService) private traceService: TraceService
) {}

@Process()
Expand All @@ -28,9 +37,15 @@ export class AuthorizationRequestProcessingConsumer {
data: job.data
})

const span = this.traceService.startSpan(`${AuthorizationRequestProcessingConsumer.name}.process`, {
attributes: { [OTEL_ATTR_JOB_ID]: job.id }
})

try {
await this.authzService.process(job.id.toString(), job.attemptsMade)
} catch (error) {
span.recordException(error)

// Short-circuits the retry mechanism on unrecoverable domain errors.
//
// IMPORTANT: To stop retrying a job in Bull, the process must return an
Expand All @@ -39,7 +54,15 @@ export class AuthorizationRequestProcessingConsumer {
return error
}

// If the error ins't recoverable, set the span status to error.
span.setStatus({
code: SpanStatusCode.ERROR,
message: error.message
})

throw error
} finally {
span.end()
}
}

Expand All @@ -66,9 +89,15 @@ export class AuthorizationRequestProcessingConsumer {

@OnQueueCompleted()
async onCompleted(job: Job<AuthorizationRequestProcessingJob>, result: unknown) {
const span = this.traceService.startSpan(`${AuthorizationRequestProcessingConsumer.name}.onCompleted`, {
attributes: { [OTEL_ATTR_JOB_ID]: job.id }
})

if (result instanceof Error) {
this.logger.error('Stop processing authorization request due to unrecoverable error', result)

span.setAttribute(OTEL_ATTR_JOB_FAILED, true).end()

await this.authzService.fail(job.id.toString(), {
...result,
id: uuid()
Expand All @@ -82,6 +111,8 @@ export class AuthorizationRequestProcessingConsumer {
data: job.data,
result
})

span.setAttribute(OTEL_ATTR_JOB_FAILED, false).end()
}

@OnQueueFailed()
Expand All @@ -93,15 +124,27 @@ export class AuthorizationRequestProcessingConsumer {
error
}

const span = this.traceService.startSpan(`${AuthorizationRequestProcessingConsumer.name}.onCompleted`, {
attributes: {
[OTEL_ATTR_JOB_ID]: log.id,
[OTEL_ATTR_JOB_MAX_RETRIES]: log.maxAttempts,
[OTEL_ATTR_JOB_ATTEMPS_MADE]: log.attemptsMade
}
})

if (job.attemptsMade >= AUTHORIZATION_REQUEST_PROCESSING_QUEUE_ATTEMPTS) {
this.logger.error('Process authorization request failed', log)
span.setAttribute(OTEL_ATTR_JOB_RETRIED, false)

await this.authzService.fail(job.id.toString(), {
id: uuid(),
...error
})
} else {
this.logger.log('Retrying to process authorization request', log)
span.setAttribute(OTEL_ATTR_JOB_RETRIED, true)
}

span.end()
}
}
Original file line number Diff line number Diff line change
@@ -1,17 +1,18 @@
import { LoggerService } from '@narval/nestjs-shared'
import { LoggerService, OTEL_ATTR_CLIENT_ID, TraceService } from '@narval/nestjs-shared'
import {
AuthorizationRequest,
AuthorizationRequestProcessingJob,
AuthorizationRequestStatus
} from '@narval/policy-engine-shared'
import { InjectQueue } from '@nestjs/bull'
import { Injectable, OnApplicationBootstrap } from '@nestjs/common'
import { Inject, Injectable, OnApplicationBootstrap } from '@nestjs/common'
import { BackoffOptions, Job, Queue } from 'bull'
import {
AUTHORIZATION_REQUEST_PROCESSING_QUEUE,
AUTHORIZATION_REQUEST_PROCESSING_QUEUE_ATTEMPTS,
AUTHORIZATION_REQUEST_PROCESSING_QUEUE_BACKOFF
} from '../../../armory.constant'
import { OTEL_ATTR_JOB_ID } from '../../orchestration.constant'
import { AuthorizationRequestRepository } from '../../persistence/repository/authorization-request.repository'

type JobOption = {
Expand All @@ -30,11 +31,16 @@ export class AuthorizationRequestProcessingProducer implements OnApplicationBoot
@InjectQueue(AUTHORIZATION_REQUEST_PROCESSING_QUEUE)
private processingQueue: Queue<AuthorizationRequestProcessingJob>,
private authzRequestRepository: AuthorizationRequestRepository,
private logger: LoggerService
private logger: LoggerService,
@Inject(TraceService) private traceService: TraceService
) {}

async add(authzRequest: AuthorizationRequest): Promise<Job<AuthorizationRequestProcessingJob>> {
return this.processingQueue.add(
const span = this.traceService.startSpan(`${AuthorizationRequestProcessingProducer.name}.add`, {
attributes: { [OTEL_ATTR_CLIENT_ID]: authzRequest.id }
})

const job = await this.processingQueue.add(
{
id: authzRequest.id
},
Expand All @@ -43,9 +49,15 @@ export class AuthorizationRequestProcessingProducer implements OnApplicationBoot
...DEFAULT_JOB_OPTIONS
}
)

span.setAttribute(OTEL_ATTR_JOB_ID, job.id).end()

return job
}

async bulkAdd(requests: AuthorizationRequest[]): Promise<Job<AuthorizationRequestProcessingJob>[]> {
const span = this.traceService.startSpan(`${AuthorizationRequestProcessingProducer.name}.bulkAdd`)

const jobs = requests.map(({ id }) => ({
data: { id },
opts: {
Expand All @@ -54,10 +66,16 @@ export class AuthorizationRequestProcessingProducer implements OnApplicationBoot
}
}))

return this.processingQueue.addBulk(jobs)
const addedJobs = await this.processingQueue.addBulk(jobs)

span.end()

return addedJobs
}

async onApplicationBootstrap() {
const span = this.traceService.startSpan(`${AuthorizationRequestProcessingProducer.name}.onApplicationBootstrap`)

const requests = await this.authzRequestRepository.findByStatus(AuthorizationRequestStatus.CREATED)

if (requests.length) {
Expand All @@ -67,5 +85,7 @@ export class AuthorizationRequestProcessingProducer implements OnApplicationBoot

await this.bulkAdd(requests)
}

span.end()
}
}
4 changes: 2 additions & 2 deletions deploy/armory.dockerfile
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
FROM node:21 as build
FROM node:20 as build

# Set the working directory
WORKDIR /usr/src/app
Expand All @@ -19,7 +19,7 @@ RUN make armory/db/generate-types && \
make armory/build && \
rm -rf apps/ && rm -rf packages/

FROM node:21 as final
FROM node:20 as final

WORKDIR /usr/src/app

Expand Down
3 changes: 3 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
"name": "@narval/source",
"version": "v0.0.13",
"license": "MPL-2.0",
"engines": {
"node": "^20.0.0"
},
"workspaces": [
"packages/*"
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ export const TraceService = Symbol('TraceService')
/**
* OpenTelemetry Trace/Span Name Conventions
*
* Format: <operation_type>.<entity>.<action>
* Format: `<operation_type>.<entity>.<action>`
*
* Common Operation Types:
* - http: Web operations
Expand Down
Loading