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

Image Render: Support Tracing #586

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
d974217
Add tracing
evictorero Aug 2, 2024
eb1db17
changed url ingestion and tried to extract traceparent
evictorero Aug 5, 2024
bb8a21a
removing unused code
evictorero Aug 6, 2024
1a0ee72
remove unused packages and add logs
evictorero Aug 7, 2024
c8ed844
add tracing info to logs and tracing configuration
evictorero Aug 7, 2024
4ff2f6c
Merge branch 'master' into poc-tracing
evictorero Aug 9, 2024
7f10f00
Merge remote-tracking branch 'origin/master' into lucychen/add_tracing
lucychen-grafana Dec 10, 2024
36b1891
fix config override
lucychen-grafana Jan 10, 2025
9eaa8cf
fix argv.config
lucychen-grafana Jan 10, 2025
a741f58
fix trace initialization and starting
lucychen-grafana Jan 29, 2025
71445a8
remove custom log format
lucychen-grafana Jan 29, 2025
37cfaa7
update json
lucychen-grafana Jan 29, 2025
45f4968
change url to env
lucychen-grafana Jan 30, 2025
18e1d25
Merge branch 'master' into lucychen/add_tracing
AgnesToulet Feb 3, 2025
3f191e2
Apply suggestions from code review
lucychen-grafana Feb 3, 2025
1eca03d
fix yarn file
lucychen-grafana Feb 3, 2025
575c323
prettier lint
lucychen-grafana Feb 3, 2025
dcd3967
add custom trace for browser
lucychen-grafana Feb 12, 2025
87f42d2
Merge branch 'lucychen/add_tracing' of https://github.com/grafana/gra…
AgnesToulet Feb 14, 2025
987f0e7
propagate traces back to Grafana
AgnesToulet Feb 17, 2025
837f072
Add tracing to plugin mode and improve config management (#602)
AgnesToulet Feb 20, 2025
4d7f051
update config for docker
lucychen-grafana Feb 20, 2025
8e94864
add to readme
lucychen-grafana Feb 20, 2025
6bc6e9b
update readme
lucychen-grafana Feb 20, 2025
4842979
update readme
lucychen-grafana Feb 21, 2025
57d7d5e
prettier
lucychen-grafana Feb 21, 2025
e2c0cef
update tracing
lucychen-grafana Feb 21, 2025
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
4 changes: 4 additions & 0 deletions default.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@
}
},

"tracing": {
"enabled": false
},

"security": {
"authToken": "-"
}
Expand Down
4 changes: 4 additions & 0 deletions dev.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@
"json": false,
"colorize": true
}
},

"tracing": {
"enabled": true
}
},
"rendering": {
Expand Down
4 changes: 4 additions & 0 deletions devenv/docker/custom-config/config.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@
"json": true,
"colorize": false
}
},

"tracing": {
"enabled": false
}
},
"rendering": {
Expand Down
8 changes: 7 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@
"@grpc/grpc-js": "^1.8.22",
"@grpc/proto-loader": "^0.7.2",
"@hapi/boom": "^10.0.0",
"@opentelemetry/api": "^1.9.0",
"@opentelemetry/auto-instrumentations-node": "^0.49.0",
"@opentelemetry/exporter-trace-otlp-http": "^0.52.1",
"@opentelemetry/resources": "^1.25.1",
"@opentelemetry/sdk-node": "^0.52.1",
"@opentelemetry/semantic-conventions": "^1.25.1",
"@puppeteer/browsers": "^2.3.1",
"chokidar": "^3.5.2",
"dompurify": "^2.5.4",
Expand All @@ -54,7 +60,7 @@
"@types/jest": "^29.5.12",
"@types/jsdom": "20.0.0",
"@types/multer": "1.4.7",
"@types/node": "^18.7.18",
"@types/node": "^22.1.0",
"@types/pixelmatch": "^5.2.6",
"@types/supertest": "^2.0.15",
"@typescript-eslint/eslint-plugin": "5.37.0",
Expand Down
5 changes: 5 additions & 0 deletions src/app.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { startTracing } from './tracing';
Copy link
Author

Choose a reason for hiding this comment

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

OpenTelemetry needs to be initialized before the other modules (express, http, winston, etc) in order for it to auto capture telemetry trace data from those libraries so it must come first

import * as path from 'path';
import * as _ from 'lodash';
import * as fs from 'fs';
Expand Down Expand Up @@ -65,6 +66,10 @@ async function main() {

const logger = new ConsoleLogger(config.service.logging);

if (config.service.tracing.enabled) {
startTracing(logger);
}

const sanitizer = createSanitizer();
const server = new HttpServer(config, logger, sanitizer);
await server.start();
Expand Down
3 changes: 3 additions & 0 deletions src/logger.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as winston from 'winston';
import { LoggingConfig } from './service/config';
import {context, trace} from "@opentelemetry/api";

export interface LogWriter {
write(message, encoding);
Expand Down Expand Up @@ -45,6 +46,8 @@ export class ConsoleLogger implements Logger {
transports.push(new winston.transports.Console(options));
}

//@opentelemetry/instrumentation-winston auto inject trace-context into Winston log records

Copy link
Author

Choose a reason for hiding this comment

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

Confirm this is the case in the log output in terminal. You can check that trace_id, span_id, and trace_flags is included

this.logger = winston.createLogger({
level: config.level,
exitOnError: false,
Expand Down
8 changes: 8 additions & 0 deletions src/service/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ export interface MetricsConfig {
requestDurationBuckets: number[];
}

export interface TracesConfig {
enabled: boolean;
}

export interface ConsoleLoggerConfig {
level?: string;
json: boolean;
Expand All @@ -29,6 +33,7 @@ export interface ServiceConfig {
metrics: MetricsConfig;
logging: LoggingConfig;
security: SecurityConfig;
tracing: TracesConfig;
};
rendering: RenderingConfig;
}
Expand All @@ -50,6 +55,9 @@ export const defaultServiceConfig: ServiceConfig = {
colorize: false,
},
},
tracing: {
enabled: false,
},
security: {
authToken: '-',
},
Expand Down
3 changes: 3 additions & 0 deletions src/service/http-server.integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ const serviceConfig: ServiceConfig = {
colorize: false,
},
},
tracing: {
enabled: true,
},
security: {
authToken: '-',
},
Expand Down
41 changes: 41 additions & 0 deletions src/tracing.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import {NodeSDK} from '@opentelemetry/sdk-node';
Copy link
Author

Choose a reason for hiding this comment

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

I followed OpenTelemetry Doc for Node.js to implement this

Copy link
Contributor

Choose a reason for hiding this comment

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

This whole file should be formatted using Prettier linter (we don't have a check for that in CI but we should do our best).

import {getNodeAutoInstrumentations} from '@opentelemetry/auto-instrumentations-node';
import {OTLPTraceExporter} from '@opentelemetry/exporter-trace-otlp-http';
import {SEMRESATTRS_SERVICE_NAME} from '@opentelemetry/semantic-conventions';
import {Resource} from '@opentelemetry/resources';
import {Logger} from "./logger";

// For troubleshooting, set the log level to DiagLogLevel.DEBUG
// const { diag, DiagConsoleLogger, DiagLogLevel } = require('@opentelemetry/api');
// diag.setLogger(new DiagConsoleLogger(), DiagLogLevel.DEBUG);

const traceExporter = new OTLPTraceExporter({
url: process.env['OTEL_EXPORTER_OTLP_TRACES_ENDPOINT'],
});

const sdk = new NodeSDK({
resource: new Resource({
[SEMRESATTRS_SERVICE_NAME]: 'image-renderer-service',
}),
traceExporter,
instrumentations: [
getNodeAutoInstrumentations({
// only instrument fs if it is part of another trace
'@opentelemetry/instrumentation-fs': {
Copy link
Author

@lucychen-grafana lucychen-grafana Jan 30, 2025

Choose a reason for hiding this comment

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

the fs module is very noisy. It create 10+ traces with singular span so it did not make much sense to have it unless it's part of another trace.

There is a list of supported autoinstrumentation. We can opt out of any in the future we do not care to trace those libraries

requireParentSpan: true,
},
})
],
});

export function startTracing(log: Logger) {
sdk.start();
log.info('Starting tracing');

process.on('SIGTERM', () => {
sdk.shutdown()
.then(() => log.debug('Tracing terminated'))
.catch((error) => log.error('Error terminating tracing', error))
.finally(() => process.exit(0));
});
Copy link
Author

Choose a reason for hiding this comment

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

Create startTracing separately from the rest of initialization steps b/c we want to only capture traces if it's enabled

}
Loading