-
Notifications
You must be signed in to change notification settings - Fork 22
feat(opentelemetry): create otel instrumentation for typed-express-router #1044
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
feat(opentelemetry): create otel instrumentation for typed-express-router #1044
Conversation
9c177cd to
0bff27e
Compare
packages/opentelemetry-instrumentation-typed-express-router/test/instrumentation.test.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-instrumentation-typed-express-router/test/instrumentation.test.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-instrumentation-typed-express-router/src/version.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-instrumentation-typed-express-router/src/utils.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts
Fixed
Show fixed
Hide fixed
43daf20 to
3781856
Compare
packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-express/src/terinstrumentation.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-express/src/terinstrumentation.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-express/src/terinstrumentation.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-express/src/terinstrumentation.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-express/src/terinstrumentation.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-express/src/terinstrumentation.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-express/src/terinstrumentation.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-express/src/terinstrumentation.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-express/test/fixtures/use-express.mjs
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-typed-express-router/src/instrumentation.ts
Fixed
Show fixed
Hide fixed
6cd264b to
11fc7f0
Compare
f804fb4 to
0158ca3
Compare
a224511 to
ab8a123
Compare
3cfad4e to
4248ffd
Compare
ericcrosson-bitgo
left a comment
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.
Apparently this text box can't be empty.
| onEncodeError: (_err, _req, res) => { | ||
| res.status(500).json('Custom encode error').end(); | ||
| encodeErrorFormatter: (_err, _req) => { | ||
| return 'Custom encode error'; |
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.
praise: Look at all that data instead of state! 😍
| body?: any, | ||
| ): Promise<{ statusCode: number; data: string }> => { | ||
| return new Promise((resolve, reject) => { | ||
| const url = `http://localhost:${(server.address() as any).port}${path}`; |
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.
question: LSP says server.address() is of type string | AddressInfo | null -- how do you always know it is of type AddressInfo here? I wonder if there's a way to clean up the type cast.
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.
According to the documentation: https://github.com/nodejs/node/blob/main/doc/api/net.md#serveraddress
For a server listening on a pipe or Unix domain socket, the name is returned
and
server.address() returns null before the 'listening' event has been emitted or after calling server.close().
Since we're listening on a port instead of a pipe or a Unix domain socket, we can at least be sure that it'll at least be of type AddressInfo | null
IDEALLY, makeRequest should only be called when the server is listening. The server starts listening before every test (as defined in beforeEach).
So I think the best option here would just be to reject the promise if it's a string or null. (This would be an impossible case for the test)
Something like:
const address = server.address();
if (typeof address === 'string' || address === null) {
reject('Unexpected address value');
return;
}
const url = `http://localhost:${address.port}${path}`;
090eaed to
08e1817
Compare
…uter This commit implements `opentelemetry` instrumentation for the `decode` and `sendEncoded` calls in `wrapRouter`. Changes: - `typed-express-wrapper` - `@opentelemetry/api@^1.0.0` added as an optional peer dependency - `@opentelemetry/[email protected]` , `@opentelemetry/[email protected]`, and `@opentelemetry/[email protected]` added as dev dependencies - `wrapRouter` modified to create decode and encode spans if `@opentelemetry/api` is installed - if `@opentelemetry/api` is not installed, spans are not created - `onDecodeError` option removed. Please use `decodeErrorFormatter` and `getDecodeErrorStatusCode` - `decodeErrorFormatter` takes in an array of `ValidationError`s and a `WrappedRequest`, returning a `Json` object. - `getDecodeErrorStatusCode` takes in an array of `ValidationError`s and a `WrappedRequest`, returning a number. - `onEncodeError` option removed. Please use `encodeErrorFormatter` and `getEncodeErrorStatusCode` - `encodeErrorFormatter` takes in an error and a `WrappedRequest`, returning a `Json` object. - `getEncodeErrorStatusCode` takes in an error and a `WrappedRequest`, returning a number. - `typed-express-router` now handles the sending of the http response when there is a decode or encode error - `express-wrapper` - `routerForApiSpec` modified to pass in new parameters `decodeErrorFormatter`, `getDecodeErrorStatusCode`, `encodeErrorFormatter`, and `getEncodeErrorStatusCode` - These new parameters can also be customized by modifying the props passed into the function (see `CreateRouterProps`) BREAKING CHANGE: `onDecodeError` and `onEncodeError` have been removed. Please use `decodeErrorFormatter`, `getDecodeErrorStatusCode`, `encodeErrorFormatter`, and `getEncodeErrorStatusCode`
08e1817 to
296a92e
Compare
ericcrosson-bitgo
left a comment
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.
Adds opentelemetry support for typed-express-router
Exemplary work, @starfy84!! 🚀
|
Double-checked that beta and master still point to the same commit: |
|
🎉 This PR is included in version @api-ts/[email protected] 🎉 The release is available on npm package (@beta dist-tag) Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version @api-ts/[email protected] 🎉 The release is available on npm package (@beta dist-tag) Your semantic-release bot 📦🚀 |
|
@starfy84 Can we update README.md on deprecation of |
Will do! Thanks for letting me know. |
|
🎉 This PR is included in version @api-ts/[email protected] 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version @api-ts/[email protected] 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version @api-ts/[email protected] 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
Ticket: DX-1473
This PR implements
opentelemetryinstrumentation for thedecodeandsendEncodedcalls inwrapRouter.Changes:
typed-express-wrapper@opentelemetry/api@^1.0.0added as an optional peer dependency@opentelemetry/[email protected],@opentelemetry/[email protected], and@opentelemetry/[email protected]added as dev dependencieswrapRoutermodified to create decode and encode spans if@opentelemetry/apiis installed@opentelemetry/apiis not installed, spans are not createdonDecodeErroroption removed. Please usedecodeErrorFormatterandgetDecodeErrorStatusCodedecodeErrorFormattertakes in an array ofValidationErrors and aWrappedRequest, returning aJsonobject.getDecodeErrorStatusCodetakes in an array ofValidationErrors and aWrappedRequest, returning a number.onEncodeErroroption removed. Please useencodeErrorFormatterandgetEncodeErrorStatusCodeencodeErrorFormattertakes in an error and aWrappedRequest, returning aJsonobject.getEncodeErrorStatusCodetakes in an error and aWrappedRequest, returning a number.typed-express-routernow handles the sending of the http response when there is a decode or encode errorexpress-wrapperrouterForApiSpecmodified to pass in new parametersdecodeErrorFormatter,getDecodeErrorStatusCode,encodeErrorFormatter, andgetEncodeErrorStatusCodeCreateRouterProps)BREAKING CHANGE:
onDecodeErrorandonEncodeErrorhave been removed. Please usedecodeErrorFormatter,getDecodeErrorStatusCode,encodeErrorFormatter, andgetEncodeErrorStatusCode