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

fix(core): Respect prefix for all Prometheus metrics #10130

Merged
merged 7 commits into from
Jul 22, 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
10 changes: 10 additions & 0 deletions packages/cli/BREAKING-CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,16 @@

This list shows all the versions which include breaking changes and how to upgrade.

## 1.52.0

### What changed?

Prometheus metrics enabled via `N8N_METRICS_INCLUDE_DEFAULT_METRICS` and `N8N_METRICS_INCLUDE_API_ENDPOINTS` were fixed to include the default `n8n_` prefix.

### When is action necessary?

If you are using Prometheus metrics from these categories and are using a non-empty prefix, please update those metrics to match their new prefixed names.

## 1.47.0

### What changed?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ describe('PrometheusMetricsService', () => {
expect(service.counters.cacheUpdatesTotal?.inc).toHaveBeenCalledWith(0);
});

it('should set up API metrics with `express-prom-bundle`', async () => {
it('should set up route metrics with `express-prom-bundle`', async () => {
config.set('endpoints.metrics.includeApiEndpoints', true);
config.set('endpoints.metrics.includeApiPathLabel', true);
config.set('endpoints.metrics.includeApiMethodLabel', true);
Expand Down
41 changes: 33 additions & 8 deletions packages/cli/src/metrics/prometheus-metrics.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@ import { CacheService } from '@/services/cache/cache.service';
import { MessageEventBus } from '@/eventbus/MessageEventBus/MessageEventBus';
import { EventMessageTypeNames } from 'n8n-workflow';
import type { EventMessageTypes } from '@/eventbus';

type MetricCategory = 'default' | 'api' | 'cache' | 'logs';
import type { Includes, MetricCategory, MetricLabel } from './types';

@Service()
export class PrometheusMetricsService {
Expand All @@ -24,10 +23,10 @@ export class PrometheusMetricsService {

private readonly prefix = config.getEnv('endpoints.metrics.prefix');

private readonly includes = {
private readonly includes: Includes = {
metrics: {
default: config.getEnv('endpoints.metrics.includeDefaultMetrics'),
api: config.getEnv('endpoints.metrics.includeApiEndpoints'),
Copy link
Member

Choose a reason for hiding this comment

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

is this a breaking change 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do you mean? This is an internal implementation detail, the schema path and the env remain the same.

Copy link
Member

Choose a reason for hiding this comment

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

does this not change the metric name that gets scraped by prometheus?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's what you mean. Since we advertise this as experimental I take it this is okay to change, but I can add a mention just in case.

routes: config.getEnv('endpoints.metrics.includeApiEndpoints'),
cache: config.getEnv('endpoints.metrics.includeCacheMetrics'),
logs: config.getEnv('endpoints.metrics.includeMessageEventBusMetrics'),
},
Expand Down Expand Up @@ -60,8 +59,20 @@ export class PrometheusMetricsService {
}

disableAllMetrics() {
for (const metric of Object.keys(this.includes.metrics) as MetricCategory[]) {
this.includes.metrics[metric] = false;
for (const metric in this.includes.metrics) {
this.includes.metrics[metric as MetricCategory] = false;
}
}

enableLabels(labels: MetricLabel[]) {
for (const label of labels) {
this.includes.labels[label] = true;
}
}

disableAllLabels() {
for (const label in this.includes.labels) {
this.includes.labels[label as MetricLabel] = false;
}
}

Expand Down Expand Up @@ -98,7 +109,7 @@ export class PrometheusMetricsService {
* Set up metrics for server routes with `express-prom-bundle`
*/
private initRouteMetrics(app: express.Application) {
if (!this.includes.metrics.api) return;
if (!this.includes.metrics.routes) return;

const metricsMiddleware = promBundle({
autoregister: false,
Expand Down Expand Up @@ -126,11 +137,25 @@ export class PrometheusMetricsService {
private mountMetricsEndpoint(app: express.Application) {
app.get('/metrics', async (_req: express.Request, res: express.Response) => {
const metrics = await promClient.register.metrics();
const prefixedMetrics = this.addPrefixToMetrics(metrics);
res.setHeader('Content-Type', promClient.register.contentType);
res.send(metrics).end();
res.send(prefixedMetrics).end();
});
}

private addPrefixToMetrics(metrics: string) {
Copy link
Member

Choose a reason for hiding this comment

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

could we make this public and add unit tests just for this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is covered by integration tests containing the n8n_test_ prefix.

return metrics
.split('\n')
.map((rawLine) => {
const line = rawLine.trim();

if (!line || line.startsWith('#') || line.startsWith(this.prefix)) return rawLine;

return this.prefix + line;
})
.join('\n');
}

/**
* Set up cache metrics: `n8n_cache_hits_total`, `n8n_cache_misses_total`, and
* `n8n_cache_updates_total`
Expand Down
14 changes: 14 additions & 0 deletions packages/cli/src/metrics/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
export type MetricCategory = 'default' | 'routes' | 'cache' | 'logs';

export type MetricLabel =
| 'credentialsType'
| 'nodeType'
| 'workflowId'
| 'apiPath'
| 'apiMethod'
| 'apiStatusCode';

export type Includes = {
metrics: Record<MetricCategory, boolean>;
labels: Record<MetricLabel, boolean>;
};
67 changes: 65 additions & 2 deletions packages/cli/test/integration/prometheus-metrics.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ describe('Metrics', () => {

beforeEach(() => {
prometheusService.disableAllMetrics();
prometheusService.disableAllLabels();
});

it('should return n8n version', async () => {
Expand All @@ -51,7 +52,9 @@ describe('Metrics', () => {

const { version, major, minor, patch } = n8nVersion;

expect(toLines(response)).toContain(
const lines = toLines(response);

expect(lines).toContain(
`n8n_test_version_info{version="v${version}",major="${major}",minor="${minor}",patch="${patch}"} 1`,
);
});
Expand All @@ -73,7 +76,10 @@ describe('Metrics', () => {
*/
expect(response.status).toEqual(200);
expect(response.type).toEqual('text/plain');
expect(toLines(response)).toContain('nodejs_heap_space_size_total_bytes{space="read_only"} 0');

const lines = toLines(response);

expect(lines).toContain('n8n_test_nodejs_heap_space_size_total_bytes{space="read_only"} 0');
});

it('should not return default metrics if disabled', async () => {
Expand Down Expand Up @@ -146,4 +152,61 @@ describe('Metrics', () => {
expect(lines).not.toContain('n8n_test_cache_misses_total 0');
expect(lines).not.toContain('n8n_test_cache_updates_total 0');
});

it('should return route metrics if enabled', async () => {
/**
* Arrange
*/
prometheusService.enableMetric('routes');
await prometheusService.init(server.app);
await agent.get('/api/v1/workflows');

/**
* Act
*/
const response = await agent.get('/metrics');

/**
* Assert
*/
expect(response.status).toEqual(200);
expect(response.type).toEqual('text/plain');

const lines = toLines(response);

expect(lines).toContain('n8n_test_http_request_duration_seconds_count 1');
expect(lines).toContainEqual(
expect.stringContaining('n8n_test_http_request_duration_seconds_sum'),
);
expect(lines).toContainEqual(
expect.stringContaining('n8n_test_http_request_duration_seconds_bucket'),
);
});

it('should return labels in route metrics if enabled', async () => {
/**
* ARrange
*/
prometheusService.enableMetric('routes');
prometheusService.enableLabels(['apiMethod', 'apiPath', 'apiStatusCode']);
await prometheusService.init(server.app);
await agent.get('/webhook-test/some-uuid');

/**
* Act
*/
const response = await agent.get('/metrics');

/**
* Assert
*/
expect(response.status).toEqual(200);
expect(response.type).toEqual('text/plain');

const lines = toLines(response);

expect(lines).toContainEqual(expect.stringContaining('method="GET"'));
expect(lines).toContainEqual(expect.stringContaining('path="/webhook-test/some-uuid"'));
expect(lines).toContainEqual(expect.stringContaining('status_code="404"'));
});
});
Loading