feat: Implement detailed tracing #326
Conversation
|
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdded OpenTelemetry-based tracing infrastructure to the federation SDK. Created a comprehensive tracing module with decorators for instrumenting classes and methods, applied tracing across repositories and services, and exported tracing utilities from the public API. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Service as Service (e.g., EventService)
participant Decorator as `@traced` Decorator
participant OpenTelemetry as OpenTelemetry Tracer
participant Span as Active Span
Client->>Service: Call method(arg1, arg2)
Service->>Decorator: Decorator intercepts call
Decorator->>Decorator: Extract attributes via provided extractor
Decorator->>OpenTelemetry: Get active span context
OpenTelemetry-->>Span: Retrieve active span
Span->>Span: Set span attributes from extraction
Decorator->>Service: Execute original method
Service-->>Decorator: Return result
Decorator->>Span: End span / Record result
Decorator-->>Client: Return result
sequenceDiagram
participant EventEmitter as EventEmitterService
participant Handler as Event Handler
participant Tracer as OpenTelemetry Tracer
participant Span as Span Context
EventEmitter->>EventEmitter: on(eventName, originalHandler)
EventEmitter->>EventEmitter: createTracedHandler wraps originalHandler
EventEmitter->>EventEmitter: Store in handlerMap[eventName]
rect rgba(100, 200, 100, 0.5)
Note over EventEmitter: Event Emission Flow
EventEmitter->>Tracer: emit(eventName, data)
Tracer->>Span: Start span for eventName
Span->>Handler: Execute tracedHandler
Handler->>Handler: Wrapped original logic
alt Handler succeeds
Handler-->>Span: Return result
Span->>Span: Set success status
else Handler throws
Handler-->>Span: Exception
Span->>Span: Record exception
Span->>Span: Set error status
end
Span-->>EventEmitter: Span ends
end
EventEmitter->>EventEmitter: off(eventName, originalHandler)
EventEmitter->>EventEmitter: Find tracedHandler in handlerMap
EventEmitter->>EventEmitter: Unsubscribe traced version
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/federation-sdk/src/services/event-emitter.service.ts (1)
171-176: Handler unsubscription is broken due to wrapper mismatch.The
on()andonce()methods wrap the original handler intracedHandlerbefore registering it with the emitter. However,off()passes the original handler directly, which will never match the wrapped handler stored in the emitter.This means
off(event, handler)will silently fail to unsubscribe when tracing is active.🐛 Suggested fix: maintain handler mapping
@singleton() export class EventEmitterService { private emitter: AsyncDispatcher<HomeserverEventSignatures> = new AsyncDispatcher<HomeserverEventSignatures>(); private tracer = trace.getTracer('@rocket.chat/federation-sdk'); + + // Map original handlers to their traced wrappers for proper cleanup + private handlerMap = new WeakMap<Function, Function>(); public on<K extends keyof HomeserverEventSignatures>( event: K, handler: EventHandlerOf<HomeserverEventSignatures, K>, ): (() => void) | undefined { const tracedHandler = async ( // ... existing implementation ): Promise<unknown> => { // ... existing implementation }; + this.handlerMap.set(handler, tracedHandler); return this.emitter.on( event, tracedHandler as EventHandlerOf<HomeserverEventSignatures, K>, ); } public off<K extends keyof HomeserverEventSignatures>( event: K, handler: EventHandlerOf<HomeserverEventSignatures, K>, ): void { - this.emitter.off(event, handler); + const tracedHandler = this.handlerMap.get(handler) ?? handler; + this.emitter.off(event, tracedHandler as EventHandlerOf<HomeserverEventSignatures, K>); + this.handlerMap.delete(handler); }
🤖 Fix all issues with AI agents
In @packages/federation-sdk/src/services/event-emitter.service.ts:
- Around line 8-12: Remove the unused Emitter import and fix import ordering:
delete the `Emitter` import from the top imports and reorder the named imports
from `@opentelemetry/api` into alphabetical order (e.g., `SpanStatusCode`,
`context`, `trace`) so the import line for `@opentelemetry/api` is alphabetized;
keep other imports (like `HomeserverEventSignatures` and
`extractEventEmitterAttributes`) intact.
In @packages/federation-sdk/src/utils/tracing.ts:
- Around line 178-190: sanitizeArguments can return objects but the attributes
cast passed to tracerActiveSpan claims only primitives; convert any
non-primitive parameter values to strings before assigning attributes.parameters
so OpenTelemetry receives only string|number|boolean|undefined. Update the code
that sets attributes.parameters (used with tracerActiveSpan) to iterate the
sanitizeArguments result and JSON.stringify or use a safe stringify for values
where typeof value === 'object' (or value is null) and leave primitives as-is,
then cast to Record<string, string|number|boolean|undefined>.
🧹 Nitpick comments (4)
packages/federation-sdk/src/services/event-emitter.service.ts (1)
72-109: Consider extracting duplicated traced handler logic.The
tracedHandlerimplementations inon()andonce()are nearly identical, differing only in thehandler.typeattribute. A helper method could reduce duplication.♻️ Optional refactor
private createTracedHandler<K extends keyof HomeserverEventSignatures>( event: K, handler: EventHandlerOf<HomeserverEventSignatures, K>, handlerType: 'on' | 'once', ): (data: EventOf<HomeserverEventSignatures, K>) => Promise<unknown> { return async (data) => { const currentSpan = trace.getSpan(context.active()); if (currentSpan) { return this.tracer.startActiveSpan( `homeserver-sdk event handler ${event}`, { attributes: { 'event.type': event as string, 'handler.type': handlerType, }, }, async (span) => { try { return await (handler as (data: unknown) => unknown)(data); } catch (err) { span.recordException(err as Error); span.setStatus({ code: SpanStatusCode.ERROR, message: err instanceof Error ? err.message : String(err), }); throw err; } finally { span.end(); } }, ); } return (handler as (data: unknown) => unknown)(data); }; }Also applies to: 126-163
packages/federation-sdk/src/utils/tracing.ts (2)
37-56: Consider recursive sanitization for nested objects and arrays.The current implementation only limits top-level object keys. Nested objects and arrays are passed through unchanged, which could still result in large trace attributes. However, this is a reasonable trade-off for simplicity in an initial implementation.
155-162: Move hardcoded method exclusions toignoreMethods.The hardcoded skip list creates implicit coupling. Consider moving these to a default
ignoreMethodslist or documenting them as always-excluded internal methods.♻️ Suggested refactor
export function traceInstanceMethods<T extends object>( instance: T, options: ITraceInstanceMethodsOptions, ): T { + const defaultIgnoredMethods = [ + 'doNotMixInclusionAndExclusionFields', + 'ensureDefaultFields', + ]; const { type, className, - ignoreMethods = [], + ignoreMethods = defaultIgnoredMethods, attributeExtractors = {}, } = options; return new Proxy(instance, { get(target, prop: string): unknown { const value = (target as Record<string, unknown>)[prop]; if (typeof value === 'function' && !ignoreMethods.includes(prop)) { return new Proxy(value as CallableFunction, { apply: (fn, thisArg, argumentsList): unknown => { - // Skip internal/utility methods - if ( - [ - 'doNotMixInclusionAndExclusionFields', - 'ensureDefaultFields', - ].includes(prop) - ) { - return Reflect.apply(fn, thisArg, argumentsList); - } - // Build attributes: start with base infopackages/federation-sdk/src/utils/tracing-attributes.ts (1)
202-215: Consider whetherreasonshould be included in traces.The ban reason (line 213) could contain user-generated content. If tracing data is exposed to broader audiences or stored long-term, consider whether this is appropriate for your compliance requirements. Otherwise, this is fine for debugging purposes.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
packages/federation-sdk/package.jsonpackages/federation-sdk/src/index.tspackages/federation-sdk/src/repositories/event-staging.repository.tspackages/federation-sdk/src/repositories/event.repository.tspackages/federation-sdk/src/repositories/key.repository.tspackages/federation-sdk/src/repositories/lock.repository.tspackages/federation-sdk/src/repositories/room.repository.tspackages/federation-sdk/src/repositories/server.repository.tspackages/federation-sdk/src/repositories/state-graph.repository.tspackages/federation-sdk/src/repositories/upload.repository.tspackages/federation-sdk/src/services/edu.service.tspackages/federation-sdk/src/services/event-emitter.service.tspackages/federation-sdk/src/services/event.service.tspackages/federation-sdk/src/services/federation-request.service.tspackages/federation-sdk/src/services/federation-validation.service.tspackages/federation-sdk/src/services/federation.service.tspackages/federation-sdk/src/services/invite.service.tspackages/federation-sdk/src/services/media.service.tspackages/federation-sdk/src/services/message.service.tspackages/federation-sdk/src/services/room.service.tspackages/federation-sdk/src/services/state.service.tspackages/federation-sdk/src/utils/tracing-attributes.tspackages/federation-sdk/src/utils/tracing.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/core/src/utils/fetch.ts:12-47
Timestamp: 2025-09-14T13:30:07.786Z
Learning: In the RocketChat/homeserver codebase, the team prefers to keep implementations simple and functional rather than adding complex optimizations prematurely. Performance improvements are deferred to future work when there's clear evidence they're needed.
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/federation-sdk/src/repositories/state-graph.repository.tspackages/federation-sdk/src/repositories/room.repository.tspackages/federation-sdk/src/services/message.service.tspackages/federation-sdk/src/services/room.service.ts
🧬 Code graph analysis (13)
packages/federation-sdk/src/repositories/state-graph.repository.ts (2)
packages/federation-sdk/src/utils/tracing.ts (1)
traceInstanceMethods(137-202)packages/federation-sdk/src/index.ts (1)
traceInstanceMethods(74-74)
packages/federation-sdk/src/utils/tracing.ts (2)
packages/federation-sdk/src/index.ts (5)
ITraceInstanceMethodsOptions(77-77)tracerActiveSpan(75-75)traceInstanceMethods(74-74)addSpanAttributes(73-73)hasActiveSpan(76-76)packages/room/src/manager/room-state.ts (1)
name(65-74)
packages/federation-sdk/src/repositories/server.repository.ts (2)
packages/federation-sdk/src/utils/tracing.ts (1)
traceInstanceMethods(137-202)packages/federation-sdk/src/index.ts (1)
traceInstanceMethods(74-74)
packages/federation-sdk/src/services/media.service.ts (2)
packages/federation-sdk/src/utils/tracing.ts (1)
traceInstanceMethods(137-202)packages/federation-sdk/src/utils/tracing-attributes.ts (1)
mediaServiceAttributeExtractors(570-579)
packages/federation-sdk/src/repositories/room.repository.ts (2)
packages/federation-sdk/src/utils/tracing.ts (1)
traceInstanceMethods(137-202)packages/federation-sdk/src/index.ts (1)
traceInstanceMethods(74-74)
packages/federation-sdk/src/services/message.service.ts (2)
packages/federation-sdk/src/utils/tracing.ts (2)
traceInstanceMethods(137-202)addSpanAttributes(222-229)packages/federation-sdk/src/utils/tracing-attributes.ts (1)
messageServiceAttributeExtractors(10-142)
packages/federation-sdk/src/repositories/event-staging.repository.ts (1)
packages/federation-sdk/src/utils/tracing.ts (1)
traceInstanceMethods(137-202)
packages/federation-sdk/src/services/room.service.ts (3)
packages/federation-sdk/src/utils/tracing.ts (1)
traceInstanceMethods(137-202)packages/federation-sdk/src/index.ts (1)
traceInstanceMethods(74-74)packages/federation-sdk/src/utils/tracing-attributes.ts (1)
roomServiceAttributeExtractors(147-248)
packages/federation-sdk/src/services/event-emitter.service.ts (3)
packages/core/src/AsyncDispatcher.ts (3)
AsyncDispatcher(92-153)EventOf(29-32)EventHandlerOf(35-40)packages/federation-sdk/src/index.ts (1)
HomeserverEventSignatures(80-146)packages/federation-sdk/src/utils/tracing-attributes.ts (1)
extractEventEmitterAttributes(613-712)
packages/federation-sdk/src/services/state.service.ts (3)
packages/federation-sdk/src/utils/tracing.ts (1)
traceInstanceMethods(137-202)packages/federation-sdk/src/index.ts (1)
traceInstanceMethods(74-74)packages/federation-sdk/src/utils/tracing-attributes.ts (1)
stateServiceAttributeExtractors(309-370)
packages/federation-sdk/src/services/edu.service.ts (3)
packages/federation-sdk/src/utils/tracing.ts (1)
traceInstanceMethods(137-202)packages/federation-sdk/src/index.ts (1)
traceInstanceMethods(74-74)packages/federation-sdk/src/utils/tracing-attributes.ts (1)
eduServiceAttributeExtractors(584-602)
packages/federation-sdk/src/services/federation.service.ts (2)
packages/federation-sdk/src/utils/tracing.ts (1)
traceInstanceMethods(137-202)packages/federation-sdk/src/utils/tracing-attributes.ts (1)
federationServiceAttributeExtractors(375-514)
packages/federation-sdk/src/utils/tracing-attributes.ts (2)
packages/federation-sdk/src/utils/tracing.ts (1)
ITraceInstanceMethodsOptions(7-32)packages/federation-sdk/src/services/message.service.ts (1)
FileMessageContent(33-51)
🪛 GitHub Actions: my-workflow
packages/federation-sdk/src/services/event-emitter.service.ts
[error] 1-1: Import statements differs from the output.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (33)
packages/federation-sdk/src/services/federation-validation.service.ts (1)
64-68: LGTM! Good optimization to avoid unnecessary network calls.The early return for same-server domain is a sensible optimization. This prevents redundant network requests when validating federation with the local server.
packages/federation-sdk/package.json (1)
19-19: LGTM!The
@opentelemetry/apidependency is correctly added to enable tracing instrumentation across the SDK. This is the standard stable API package for OpenTelemetry in JavaScript/TypeScript.packages/federation-sdk/src/repositories/server.repository.ts (1)
19-25: LGTM!The tracing proxy wrapper is correctly implemented. The
biome-ignorecomment appropriately documents the intentional constructor return pattern used for tracing instrumentation.packages/federation-sdk/src/services/room.service.ts (1)
70-77: LGTM!The tracing integration is well-implemented with custom attribute extractors for key room operations. Methods without explicit extractors will gracefully fall back to sanitized parameter logging.
packages/federation-sdk/src/repositories/room.repository.ts (1)
24-30: LGTM!The tracing proxy wrapper follows the same consistent pattern used across other repository classes in this PR.
packages/federation-sdk/src/services/edu.service.ts (1)
23-30: LGTM!The tracing integration provides complete coverage for both service methods with appropriate attribute extractors. The extractors properly capture operation metadata (counts, IDs) while avoiding logging of potentially sensitive presence content.
packages/federation-sdk/src/services/state.service.ts (1)
28-29: LGTM!The tracing integration follows the established pattern consistently. The
stateServiceAttributeExtractorscorrectly maps to the public methods ofStateService, enriching traces with contextual data likeroomId,eventId, andeventType.Also applies to: 83-90
packages/federation-sdk/src/repositories/key.repository.ts (1)
3-3: LGTM!Clean integration of tracing for the repository. The default parameter sanitization fallback is appropriate here since the repository methods have straightforward signatures.
Also applies to: 16-22
packages/federation-sdk/src/repositories/event.repository.ts (1)
21-21: LGTM!The tracing wrapper is correctly applied. Given the number of methods in this repository, the default parameter sanitization approach is pragmatic - custom extractors can be added incrementally for high-value methods if needed.
Also applies to: 28-34
packages/federation-sdk/src/services/media.service.ts (1)
3-4: LGTM!The tracing integration is well-targeted. The
mediaServiceAttributeExtractorsextractsserverNameandmediaIdwhich are valuable for debugging media download issues across federated servers.Also applies to: 15-22
packages/federation-sdk/src/repositories/lock.repository.ts (1)
3-3: LGTM!The tracing wrapper is correctly positioned after the index creation. The proxy will intercept future method calls (
getLock,releaseLock,updateLockTimestamp) without interfering with the initialization logic.Also applies to: 18-22
packages/federation-sdk/src/repositories/event-staging.repository.ts (1)
20-25: LGTM! Tracing proxy wrapper correctly applied.The constructor returns a proxy-wrapped instance for method-level tracing. The index creation at lines 14-19 executes before the return, so it won't be affected by the proxy wrapper.
packages/federation-sdk/src/repositories/upload.repository.ts (1)
20-26: LGTM! Consistent tracing pattern applied.The tracing wrapper follows the same pattern as other repositories in this PR.
packages/federation-sdk/src/index.ts (1)
71-78: LGTM! Tracing utilities properly exported.The public API is extended with the necessary tracing utilities for downstream consumers. The compatibility note with
@rocket.chat/tracingprovides useful context.packages/federation-sdk/src/services/federation-request.service.ts (2)
22-79: Well-designed attribute extractors for federation request tracing.The extractors capture essential routing information (method, targetServer/domain, endpoint/uri) without logging sensitive data like request bodies or query parameters. This provides good observability while maintaining security.
109-116: LGTM! Tracing integration follows established pattern.The constructor properly applies the tracing proxy with the locally-defined attribute extractors.
packages/federation-sdk/src/services/event.service.ts (1)
66-73: LGTM! Tracing wrapper with custom attribute extractors.The service correctly applies the tracing proxy with
eventServiceAttributeExtractorsfor enriched span attributes on all key methods:getEventById,checkIfEventsExists,processIncomingTransaction,processIncomingPDUs,processIncomingEDUs, andemitEventByType. This ensures critical event processing operations are properly instrumented for observability.packages/federation-sdk/src/repositories/state-graph.repository.ts (1)
34-40: LGTM!The tracing integration follows the established pattern used across other repositories. The proxy wrapper is correctly implemented with the appropriate biome-ignore comment explaining the intentional constructor return.
packages/federation-sdk/src/services/federation.service.ts (1)
34-41: LGTM!The tracing integration is properly implemented with service-specific attribute extractors that capture relevant context (targetDomain, roomId, userId, etc.) for federation operations. The proxy wrapper pattern is consistent with other services.
packages/federation-sdk/src/services/message.service.ts (1)
62-69: LGTM!The tracing integration is well-implemented with appropriate attribute extractors. The additional
addSpanAttributescalls throughout the file effectively enrich spans with runtime-computed values (eventId, roomVersion) that aren't available at method invocation time.packages/federation-sdk/src/services/invite.service.ts (1)
43-50: LGTM!The tracing integration follows the established pattern. The repositioning of
validateOutboundInvite()to occur after the local-invite early return (lines 126-129) is a sensible optimization—outbound validation is only needed for remote users.packages/federation-sdk/src/utils/tracing.ts (4)
1-32: LGTM!The interface is well-documented and properly typed. Good use of JSDoc comments explaining the purpose of each field, especially the note about minification mangling constructor names.
67-111: LGTM!The function correctly handles both synchronous and asynchronous execution paths. The async span lifecycle is properly managed—the span ends in
.finally()after the promise settles, and exceptions are correctly recorded before the error status is set.
222-229: LGTM with note on type safety.The function gracefully handles the absence of an active span. The type cast assumes callers pass primitive values only—this is a reasonable contract for a public API.
235-237: LGTM!Simple and effective utility for conditional tracing logic.
packages/federation-sdk/src/utils/tracing-attributes.ts (8)
1-142: LGTM!The attribute extractors are well-structured with comprehensive coverage of MessageService methods. The use of optional chaining (
?.) provides good runtime safety, and thetry-catchwrapper intraceInstanceMethodsprovides a safety net for extraction failures.
250-304: LGTM!Good approach of extracting counts (
pduCount,eduCount) for bulk operations rather than full payloads. This keeps trace data lightweight while providing useful debugging context.
306-370: LGTM!Comprehensive coverage of StateService methods with appropriate attribute extraction from nested parameter objects.
372-514: LGTM!Excellent coverage of FederationService methods with consistent naming conventions (
targetDomainfor outbound federation calls). The extractors capture the right level of detail for debugging federation issues.
516-565: LGTM!Good coverage of invite service methods with useful context like
isDirectMessageflag.
567-579: LGTM!Simple and appropriate for the current MediaService scope.
581-602: LGTM!Appropriate attribute extraction for EDU service methods.
613-712: LGTM!Excellent defensive coding with thorough type guards throughout. The separation of common field extraction from event-specific handling is clean and maintainable. The explicit return type ensures only primitive values are returned to OpenTelemetry.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 24 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/federation-sdk/src/utils/tracing.ts">
<violation number="1" location="packages/federation-sdk/src/utils/tracing.ts:180">
P2: Type mismatch: `sanitizeArguments` returns an array that can contain objects, but the attributes are cast as `Record<string, string | number | boolean | undefined>`. OpenTelemetry's `setAttributes` only accepts primitive values, so passing objects will cause runtime issues or silent data loss. Consider stringifying the sanitized arguments: `attributes.parameters = JSON.stringify(sanitizeArguments(argumentsList))`.</violation>
</file>
<file name="packages/federation-sdk/src/services/event-emitter.service.ts">
<violation number="1" location="packages/federation-sdk/src/services/event-emitter.service.ts:72">
P2: Significant code duplication between `on()` and `once()` methods. The `tracedHandler` wrapper logic is duplicated (~70 lines). Extract the common wrapping logic into a private method like `createTracedHandler(event, handler, handlerType)` to improve maintainability.</violation>
<violation number="2" location="packages/federation-sdk/src/services/event-emitter.service.ts:111">
P0: Handler wrapping breaks `off()` unsubscription. The `on()` method registers a wrapped `tracedHandler`, but `off()` expects the original handler reference. This prevents users from unsubscribing handlers. Consider maintaining a WeakMap to track original-to-wrapped handler mappings, or return the wrapped handler reference.</violation>
</file>
<file name="packages/federation-sdk/src/services/room.service.ts">
<violation number="1" location="packages/federation-sdk/src/services/room.service.ts:75">
P2: The roomServiceAttributeExtractors has parameter order mismatches for kickUser, banUser, and createRoom methods. This will cause trace attributes to contain incorrect values (e.g., senderId will actually contain the target user ID). Fix the parameter destructuring order in tracing-attributes.ts to match the actual method signatures.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/federation-sdk/src/utils/tracing.ts`:
- Around line 292-299: The addSpanAttributes function currently accepts
Record<string, unknown> and casts it unsafely before calling
trace.getActiveSpan() and span.setAttributes; update addSpanAttributes to either
tighten its parameter type to Record<string, string | number | boolean |
undefined> or (recommended) perform runtime sanitization like sanitizeArguments:
iterate the incoming attributes, keep only values that are
string/number/boolean/undefined, and pass that sanitized record to
span.setAttributes; reference addSpanAttributes, trace.getActiveSpan,
span.setAttributes, and reuse the sanitizeArguments approach for validation and
filtering.
♻️ Duplicate comments (4)
packages/federation-sdk/src/services/event-emitter.service.ts (3)
71-114: Significant code duplication betweenon()andonce().The
tracedHandlerwrapper logic is nearly identical (~40 lines), differing only in thehandler.typeattribute. Extract to a private helper for maintainability.♻️ Extract common handler wrapping logic
private createTracedHandler<K extends keyof HomeserverEventSignatures>( event: K, handler: EventHandlerOf<HomeserverEventSignatures, K>, handlerType: 'on' | 'once', ): (data: EventOf<HomeserverEventSignatures, K>) => Promise<unknown> { return async (data) => { const currentSpan = trace.getSpan(context.active()); if (currentSpan) { return this.tracer.startActiveSpan( `homeserver-sdk event handler ${event}`, { attributes: { 'event.type': event as string, 'handler.type': handlerType, }, }, async (span) => { try { return await (handler as (data: unknown) => unknown)(data); } catch (err) { span.recordException(err as Error); span.setStatus({ code: SpanStatusCode.ERROR, message: err instanceof Error ? err.message : String(err), }); throw err; } finally { span.end(); } }, ); } return (handler as (data: unknown) => unknown)(data); }; }Also applies to: 125-168
10-10: Unused import:Emitter.The
Emitterimport from@rocket.chat/emitteris not used in this file.🔧 Remove unused import
-import { Emitter } from '@rocket.chat/emitter';
111-115: Critical: Handler wrapping breaksoff()unsubscription.The
on()andonce()methods register wrappedtracedHandlerfunctions, butoff()expects the original handler reference. This prevents users from unsubscribing:const handler = (data) => console.log(data); service.on('event', handler); // Registers tracedHandler service.off('event', handler); // Fails - handler !== tracedHandler🔧 Fix using a WeakMap to track handler mappings
export class EventEmitterService { private emitter: AsyncDispatcher<HomeserverEventSignatures> = new AsyncDispatcher<HomeserverEventSignatures>(); private tracer = trace.getTracer('@rocket.chat/federation-sdk'); + + // Map original handlers to their traced wrappers for proper unsubscription + private handlerMap = new WeakMap<Function, Function>(); // ... in on() method: + this.handlerMap.set(handler, tracedHandler); return this.emitter.on( event, tracedHandler as EventHandlerOf<HomeserverEventSignatures, K>, ); // ... in once() method: + this.handlerMap.set(handler, tracedHandler); return this.emitter.once( event, tracedHandler as EventHandlerOf<HomeserverEventSignatures, K>, ); public off<K extends keyof HomeserverEventSignatures>( event: K, handler: EventHandlerOf<HomeserverEventSignatures, K>, ): void { - this.emitter.off(event, handler); + const tracedHandler = this.handlerMap.get(handler as Function); + this.emitter.off(event, (tracedHandler ?? handler) as EventHandlerOf<HomeserverEventSignatures, K>); + if (tracedHandler) { + this.handlerMap.delete(handler as Function); + } }Also applies to: 171-176
packages/federation-sdk/src/utils/tracing.ts (1)
239-250: Type mismatch:sanitizeArgumentsreturns objects but attributes are cast as primitives.The
sanitizeArgumentsfunction (lines 92-115) can return objects containing nestedlimitedObjectvalues, but the cast on lines 246-249 claims all values arestring | number | boolean | undefined. OpenTelemetry'ssetAttributesexpects primitive values only—passing objects may cause runtime issues or silent data loss.🔧 Suggested fix: stringify non-primitive parameters
} else { - attributes.parameters = sanitizeArguments(args); + try { + attributes.parameters = JSON.stringify(sanitizeArguments(args)); + } catch { + attributes.parameters = '[unserializable]'; + } }
🧹 Nitpick comments (2)
packages/federation-sdk/src/services/federation-request.service.ts (1)
52-58: Consider consistent attribute naming.The
makeSignedRequestextractor usestargetDomainwhile other methods usetargetServer. For consistent trace analysis, consider aligning the attribute name.♻️ Suggested consistency fix
`@traced`( (params: { method: string; domain: string; uri: string }) => ({ method: params?.method, - targetDomain: params?.domain, + targetServer: params?.domain, uri: params?.uri, }), )packages/federation-sdk/src/services/message.service.ts (1)
424-436: Minor: Unusedemojiparameter in trace attributes.The extractor captures
emojibut the method parameter is_emoji(unused). While the tracing will work correctly, consider using underscore prefix in the extractor for consistency, or verify thatemojiis intentionally being traced despite the method not using it.Suggested consistency fix
`@traced`( ( roomId: RoomID, eventIdReactedTo: EventID, - emoji: string, + _emoji: string, senderUserId: UserID, ) => ({ roomId, eventIdReactedTo, - emoji, senderUserId, }), )
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/federation-sdk/src/services/event-emitter.service.ts`:
- Around line 27-99: The handlerMap currently maps original handler -> single
traced wrapper, so repeated subscriptions overwrite previous wrappers and off()
can remove the wrong wrapper; change handlerMap to track wrappers per event and
allow multiple wrappers per original handler (e.g., map original handler to a
Map/event-key -> Set of traced wrappers or map event -> Map<handler,
Set<wrapper>>), update subscribe (which uses createTracedHandler and sets
this.handlerMap) to add the new traced wrapper into that per-event set, and
update off() to look up and remove only the exact traced wrapper for the given
event (removing empty sets/maps as needed) so each registration can be removed
deterministically without leaking subscriptions.
… test.failing() is recognized
…nd remove bun-version-file from CI workflow
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #326 +/- ##
==========================================
- Coverage 52.30% 50.97% -1.34%
==========================================
Files 97 98 +1
Lines 13174 13966 +792
==========================================
+ Hits 6891 7119 +228
- Misses 6283 6847 +564 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.