-
Notifications
You must be signed in to change notification settings - Fork 12
feat(otel): initialize OpenTelemetry package with tracer provider and span exporter #188
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
Conversation
JakeChampion
commented
May 12, 2025
- Added package.json for @netlify/otel with necessary dependencies and scripts.
- Implemented createTracerProvider function to set up tracing within bootstrap
- Created NetlifySpanExporter class for exporting spans the spans to the console
- Defined constants for tracer retrieval and shutdown outside the bootstrap
- exports getTracer and shutdownTracers functions for use inside other packages
- Wrote tests for getTracer and shutdownTracers functionalities.
… span exporter - Added package.json for @netlify/otel with necessary dependencies and scripts. - Implemented createTracerProvider function to set up tracing within bootstrap - Created NetlifySpanExporter class for exporting spans the spans to the console - Defined constants for tracer retrieval and shutdown outside the bootstrap - exports getTracer and shutdownTracers functions for use inside other packages - Wrote tests for getTracer and shutdownTracers functionalities.
packages/otel/src/bootstrap/main.ts
Outdated
| configurable: true, | ||
| writable: false, | ||
| value: async () => { | ||
| return await nodeTracerProvider.shutdown() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be used to shutdown the open telemetry provider at the end of an invocation, thereby ensuring the lambda instance can be used again in the future without retaining any tracing information from the previous invocation -- this enables us to activate tracing for a single request and not all future requests to the same lambda instance
packages/otel/src/constants.ts
Outdated
| @@ -0,0 +1,2 @@ | |||
| export const GET_TRACER = Symbol('getTracer') | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we not need Symbol.for here? If I understand correctly, we'll have two instances of @netlify/otel: one inline into serverless-functions-api and the other one loaded as a dependency from user code. I think Symbol("getTracer") will map to two different symbols, whereas Symbol.for("getTracer") would point to the same one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's true, perhaps we should just use a string instead of adding to the global symbol registry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
packages/otel/src/main.ts
Outdated
| import { GET_TRACER, SHUTDOWN_TRACERS } from './constants.js'; | ||
|
|
||
| export const getTracer = async (name?: string, version?: string): Promise<SugaredTracer | undefined> => { | ||
| // @ts-ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid using @ts-ignore, in other places I did this:
| const requestContext = (globalThis as GlobalScope).Netlify?.context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, updated to use similar approach
packages/otel/src/bootstrap/main.ts
Outdated
| siteId: string, | ||
| siteName: string, | ||
| }) => { | ||
| if (options.headers.has('x-nf-enable-tracing')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would flip this and early-return here, to avoid having everything nested under this statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
| return | ||
| } | ||
|
|
||
| console.log('__nfOTLPTrace', NetlifySpanExporter.#decoder.decode(JsonTraceSerializer.serializeRequest(spans))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: We could move __nfOTLPTrace to the constants file.
packages/otel/src/main.ts
Outdated
| } | ||
|
|
||
| export const shutdownTracers = async (): Promise<void> => { | ||
| // @ts-ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this?