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.

Sign up for GitHub

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

Jump to bottom

Draft

starfy84 wants to merge 3 commits into master from DX-1473-create-otel-wrapper-for-typed-express-router

Contributor

starfy84 commented Jun 25, 2025 •

edited

Loading

Ticket: DX-1473

This PR implements an opentelemetry instrumentation for @api-ts/typed-express-router.

It patches the library's wrapRouter and createRouter functions and adds middleware to each request method.

To use this instrumentation, register it with the otel sdk.

starfy84 self-assigned this

starfy84 force-pushed the DX-1473-create-otel-wrapper-for-typed-express-router branch from 7b37e6f to 9c177cd Compare

June 26, 2025 14:42


          feat(opentelemetry): create otel instrumentation for

0bff27e

typed-express-router

starfy84 force-pushed the DX-1473-create-otel-wrapper-for-typed-express-router branch from 9c177cd to 0bff27e Compare

June 26, 2025 15:07

starfy84 marked this pull request as ready for review

June 26, 2025 15:15

starfy84 requested a review from a team as a code owner

June 26, 2025 15:15

ericcrosson-bitgo requested changes

View reviewed changes

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

Comment on lines 57 to 59

+              // Note: We cannot test the actual patching functionality in isolation
+              // because it requires a real typed-express-router module.
+              // Integration tests will cover this part.

Contributor

ericcrosson-bitgo Jun 27, 2025

Can you help me understand the utility provided by this test file? I see that we are checking whether or not internal values are set, but we aren't extending the assertion to cover the behavior that this modifies in an TypedExpressRouterInstrumentation instance.

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

Comment on lines +113 to +130

+              export function handleValidationErrors(span: Span, errors: Errors): void {
+                const validationErrors = PathReporter.failure(errors);
+                const validationErrorMessage = validationErrors.join('\n');
+                span.setStatus({
+                  code: SpanStatusCode.ERROR,
+                  message: 'Validation error',
+                });
+                span.setAttribute(ApiTsAttributes.API_TS_VALIDATION_ERROR, validationErrorMessage);
+                // Record exception event for better error reporting
+                span.recordException({
+                  name: 'ValidationError',
+                  message: validationErrorMessage,
+                });
+              }

Contributor

ericcrosson-bitgo Jun 27, 2025

Code coverage in this package is at 32% of statements, so I'm not sure we've sufficiently asserted that the instrumentation use cases we expose create the desired spans. This function isn't used anywhere else in this PR. Can you please create a test case for it and other functions exposed to users of this library?

We should include assertions that fix the behavior of this code, namely that this instrumenter creates spans containing the data and format we desire. This prevents future regressions and acts as a form of documentation because users of the library can be the test cases to learn from a known working context how to use this library and what benefit it provides them.

github-advanced-security bot found potential problems

View reviewed changes

packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts

+                SimpleSpanProcessor,
+              } from '@opentelemetry/sdk-trace-base';
+              import { AsyncLocalStorageContextManager } from '@opentelemetry/context-async-hooks';
+              import { SpanStatusCode, SpanKind, trace, context, Span } from '@opentelemetry/api';

Check notice

Code scanning / CodeQL

Unused variable, import, function or class Note test

Unused import SpanStatusCode.

Copilot Autofix

AI 1 day ago

To fix the issue, the unused SpanStatusCode import should be removed from the import statement on line 14. This will eliminate unnecessary clutter and improve code readability. No additional changes are required, as removing the unused import does not affect the functionality of the code.

Suggested changeset 1

packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts

@@ -13,3 +13,3 @@
             import { AsyncLocalStorageContextManager } from '@opentelemetry/context-async-hooks';
-            import { SpanStatusCode, SpanKind, trace, context, Span } from '@opentelemetry/api';
+            import { SpanKind, trace, context, Span } from '@opentelemetry/api';
             import { afterEach, before, beforeEach, describe, test } from 'node:test';

Copilot is powered by AI and may make mistakes. Always verify output.

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

+              import { apiSpec, httpRequest, httpRoute, optional } from '@api-ts/io-ts-http';
+              import { TypedRequestHandler, createRouter } from '@api-ts/typed-express-router';
+              import { ApiTsAttributes } from '../src/utils';
+              import { handleGenericError, handleValidationErrors } from '../src/utils';

Check notice

Code scanning / CodeQL

Unused variable, import, function or class Note test

Unused imports handleGenericError, handleValidationErrors.

Copilot Autofix

AI 1 day ago

To fix the issue, the unused imports handleGenericError and handleValidationErrors should be removed from the file. This will clean up the code and eliminate unnecessary dependencies. The changes should be made directly to the import statement on line 27.

Suggested changeset 1

packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts

@@ -26,3 +26,3 @@
             import { TypedRequestHandler, createRouter } from '@api-ts/typed-express-router';
-            import { handleGenericError, handleValidationErrors } from '../src/utils';
             import { Errors } from 'io-ts';

Copilot is powered by AI and may make mistakes. Always verify output.

packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts

+              import { TypedRequestHandler, createRouter } from '@api-ts/typed-express-router';
+              import { ApiTsAttributes } from '../src/utils';
+              import { handleGenericError, handleValidationErrors } from '../src/utils';
+              import { Errors } from 'io-ts';

Check notice

Code scanning / CodeQL

Unused variable, import, function or class Note test

Unused import Errors.

Copilot Autofix

AI 1 day ago

To fix the problem, the unused import Errors should be removed from the file. This will improve code readability and eliminate unnecessary clutter. The removal of the import does not affect the functionality of the code, as Errors is not referenced anywhere in the provided snippet.

Suggested changeset 1

packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts

@@ -27,3 +27,3 @@
             import { handleGenericError, handleValidationErrors } from '../src/utils';
-            import { Errors } from 'io-ts';
             import { makeRequest } from './util';

Copilot is powered by AI and may make mistakes. Always verify output.

packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts Fixed Show fixed Hide fixed


          wip: address pr comments

starfy84 force-pushed the DX-1473-create-otel-wrapper-for-typed-express-router branch from 43daf20 to 3781856 Compare

June 30, 2025 15:38


          wip: fixing tests

34d123a

github-advanced-security bot found potential problems

View reviewed changes

packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts

+              // Register instrumentation before importing modules
+              import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node';
+              import { RPCType, setRPCMetadata } from '@opentelemetry/core';

Check notice

Code scanning / CodeQL

Unused variable, import, function or class Note test

Unused imports RPCType, setRPCMetadata.

Copilot Autofix

AI 1 day ago

To fix the problem, the unused imports RPCType and setRPCMetadata should be removed from the file. This will eliminate unnecessary clutter and improve code readability. The change involves editing the import statement on line 7 to exclude these unused elements.

Suggested changeset 1

packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts

@@ -6,3 +6,3 @@
             import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node';
-            import { RPCType, setRPCMetadata } from '@opentelemetry/core';
+            import {} from '@opentelemetry/core';
             import { TypedExpressRouterInstrumentation } from '../src';

Copilot is powered by AI and may make mistakes. Always verify output.

packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts

+              import express from 'express';
+              import * as http from 'http';
+              import * as t from 'io-ts';
+              import { apiSpec, httpRequest, httpRoute, optional } from '@api-ts/io-ts-http';

Check notice

Code scanning / CodeQL

Unused variable, import, function or class Note test

Unused import optional.

Copilot Autofix

AI 1 day ago

To fix the issue, the unused optional import should be removed from the import statement on line 20. This will clean up the code and eliminate the unnecessary import. No additional changes are required since removing an unused import does not affect the functionality of the code.

Suggested changeset 1

packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts

@@ -19,3 +19,3 @@
             import * as t from 'io-ts';
-            import { apiSpec, httpRequest, httpRoute, optional } from '@api-ts/io-ts-http';
+            import { apiSpec, httpRequest, httpRoute } from '@api-ts/io-ts-http';

Copilot is powered by AI and may make mistakes. Always verify output.

ericcrosson-bitgo marked this pull request as draft

June 30, 2025 20:15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet