Conversation
|
|
WalkthroughAdds OpenTelemetry-based tracing and Prometheus metrics to the federation SDK: new tracing utilities, metrics module, exports, and instrumentation applied across repositories, services, and the event emitter. Also adds runtime dependencies and updates package manager. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service as Service (e.g., MessageService)
participant Decorator as `@traced` Decorator
participant OTEL as OpenTelemetry Tracer
participant Span as Active Span
Client->>Service: call method(args)
Service->>Decorator: decorated call
Decorator->>Decorator: extract attributes from args
Decorator->>OTEL: start span / get active context
OTEL-->>Span: return active span
Span->>Span: set attributes
Decorator->>Service: invoke original method
Service-->>Decorator: method returns / throws
Decorator->>Span: record exception/status if any
Decorator->>Span: end span
Decorator-->>Client: return result
sequenceDiagram
participant EventEmitter as EventEmitterService
participant OTEL as OpenTelemetry Tracer
participant Handler as Wrapped Handler
participant Metrics as federationMetrics
EventEmitter->>EventEmitter: on(event, originalHandler)
EventEmitter->>EventEmitter: createTracedHandler(originalHandler)
EventEmitter->>EventEmitter: store wrapper in handlerMap
rect rgba(100,200,100,0.5)
EventEmitter->>OTEL: emit(event, data)
OTEL->>OTEL: start span for event
OTEL->>Handler: call traced handler
Handler->>Handler: execute originalHandler logic
alt success
Handler-->>OTEL: returns
OTEL->>Metrics: observe duration / increment counters
else failure
Handler-->>OTEL: throws
OTEL->>OTEL: record exception / set error status
OTEL->>Metrics: increment failure counter
end
OTEL->>OTEL: end span
end
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:
|
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.service.ts (1)
30-37:⚠️ Potential issue | 🟡 MinorFix import ordering to resolve pipeline failure.
The pipeline failure indicates import statement ordering issues. The relative imports should be alphabetically ordered.
../metricsand../metrics/helpersshould come before../queues/*,../repositories/*, and../utils/*.🔧 Proposed fix
+import { federationMetrics } from '../metrics'; +import { bucketizeEduCount, bucketizePduCount } from '../metrics/helpers'; import { StagingAreaQueue } from '../queues/staging-area.queue'; import { EventStagingRepository } from '../repositories/event-staging.repository'; import { EventRepository } from '../repositories/event.repository'; import { LockRepository } from '../repositories/lock.repository'; -import { federationMetrics } from '../metrics'; -import { bucketizeEduCount, bucketizePduCount } from '../metrics/helpers'; import { eventSchemas } from '../utils/event-schemas'; import { traced, tracedClass } from '../utils/tracing';
🤖 Fix all issues with AI agents
In `@packages/federation-sdk/src/metrics/helpers.ts`:
- Around line 5-7: extractOriginFromMatrixRoomId currently uses
roomId.split(':').pop(), which returns only the port when the server name
contains a colon (e.g., matrix.org:8448); update the function to join all
segments after the first colon using roomId.split(':').slice(1).join(':') and
fall back to 'unknown' when the result is empty so the full server host:port is
returned correctly.
- Around line 13-15: The current extractOriginFromMatrixUserId implementation
uses pop on a colon-split which breaks when the server name contains additional
colons (ports); update extractOriginFromMatrixUserId to mirror the fix used in
extractOriginFromMatrixRoomId by taking everything after the first colon rather
than the last — i.e., split on ':' and join all segments after the first back
together (or use substring from the first ':' + 1) to preserve host:port, and
keep the fallback to 'unknown' if missing.
| export function extractOriginFromMatrixRoomId(roomId: string): string { | ||
| return roomId.split(':').pop() || 'unknown'; | ||
| } |
There was a problem hiding this comment.
Edge case: Matrix IDs with ports may not parse correctly.
Matrix server names can include ports (e.g., matrix.org:8448), which would cause split(':').pop() to return only the port instead of the full server name. Consider using split(':').slice(1).join(':') instead.
🔧 Proposed fix
export function extractOriginFromMatrixRoomId(roomId: string): string {
- return roomId.split(':').pop() || 'unknown';
+ const parts = roomId.split(':');
+ return parts.length > 1 ? parts.slice(1).join(':') : 'unknown';
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function extractOriginFromMatrixRoomId(roomId: string): string { | |
| return roomId.split(':').pop() || 'unknown'; | |
| } | |
| export function extractOriginFromMatrixRoomId(roomId: string): string { | |
| const parts = roomId.split(':'); | |
| return parts.length > 1 ? parts.slice(1).join(':') : 'unknown'; | |
| } |
🤖 Prompt for AI Agents
In `@packages/federation-sdk/src/metrics/helpers.ts` around lines 5 - 7,
extractOriginFromMatrixRoomId currently uses roomId.split(':').pop(), which
returns only the port when the server name contains a colon (e.g.,
matrix.org:8448); update the function to join all segments after the first colon
using roomId.split(':').slice(1).join(':') and fall back to 'unknown' when the
result is empty so the full server host:port is returned correctly.
| export function extractOriginFromMatrixUserId(userId: string): string { | ||
| return userId.split(':').pop() || 'unknown'; | ||
| } |
There was a problem hiding this comment.
Same port edge case applies here.
Apply the same fix as extractOriginFromMatrixRoomId to handle server names with ports.
🔧 Proposed fix
export function extractOriginFromMatrixUserId(userId: string): string {
- return userId.split(':').pop() || 'unknown';
+ const parts = userId.split(':');
+ return parts.length > 1 ? parts.slice(1).join(':') : 'unknown';
}🤖 Prompt for AI Agents
In `@packages/federation-sdk/src/metrics/helpers.ts` around lines 13 - 15, The
current extractOriginFromMatrixUserId implementation uses pop on a colon-split
which breaks when the server name contains additional colons (ports); update
extractOriginFromMatrixUserId to mirror the fix used in
extractOriginFromMatrixRoomId by taking everything after the first colon rather
than the last — i.e., split on ':' and join all segments after the first back
together (or use substring from the first ':' + 1) to preserve host:port, and
keep the fallback to 'unknown' if missing.
There was a problem hiding this comment.
3 issues found across 7 files (changes from recent commits).
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/services/event.service.ts">
<violation number="1" location="packages/federation-sdk/src/services/event.service.ts:145">
P1: The new try/finally wraps the “too-many-concurrent-transactions” guard, so a rejected request still runs `finally` and deletes the existing `origin` entry. That clears an in-flight transaction and allows concurrent processing that should be blocked.</violation>
</file>
<file name="packages/federation-sdk/src/metrics/helpers.ts">
<violation number="1" location="packages/federation-sdk/src/metrics/helpers.ts:6">
P2: Origin extraction uses the substring after the last colon. Matrix IDs should be split at the first colon per spec, so IDs containing additional colons (e.g., server name with port) will produce incorrect origins. Consider extracting everything after the first colon instead.</violation>
<violation number="2" location="packages/federation-sdk/src/metrics/helpers.ts:14">
P2: Origin extraction uses the substring after the last colon. Matrix IDs should be split at the first colon per spec, so IDs containing additional colons (e.g., server name with port) will produce incorrect origins. Consider extracting everything after the first colon instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (edus && !Array.isArray(edus)) { | ||
| throw new Error('edus must be an array'); | ||
| } | ||
| try { |
There was a problem hiding this comment.
P1: The new try/finally wraps the “too-many-concurrent-transactions” guard, so a rejected request still runs finally and deletes the existing origin entry. That clears an in-flight transaction and allows concurrent processing that should be blocked.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/federation-sdk/src/services/event.service.ts, line 145:
<comment>The new try/finally wraps the “too-many-concurrent-transactions” guard, so a rejected request still runs `finally` and deletes the existing `origin` entry. That clears an in-flight transaction and allows concurrent processing that should be blocked.</comment>
<file context>
@@ -133,27 +135,34 @@ export class EventService {
- if (edus && !Array.isArray(edus)) {
- throw new Error('edus must be an array');
- }
+ try {
+ if (!Array.isArray(pdus)) {
+ throw new Error('pdus must be an array');
</file context>
| * @example extractOriginFromMatrixUserId('@user:matrix.org') // 'matrix.org' | ||
| */ | ||
| export function extractOriginFromMatrixUserId(userId: string): string { | ||
| return userId.split(':').pop() || 'unknown'; |
There was a problem hiding this comment.
P2: Origin extraction uses the substring after the last colon. Matrix IDs should be split at the first colon per spec, so IDs containing additional colons (e.g., server name with port) will produce incorrect origins. Consider extracting everything after the first colon instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/federation-sdk/src/metrics/helpers.ts, line 14:
<comment>Origin extraction uses the substring after the last colon. Matrix IDs should be split at the first colon per spec, so IDs containing additional colons (e.g., server name with port) will produce incorrect origins. Consider extracting everything after the first colon instead.</comment>
<file context>
@@ -0,0 +1,87 @@
+ * @example extractOriginFromMatrixUserId('@user:matrix.org') // 'matrix.org'
+ */
+export function extractOriginFromMatrixUserId(userId: string): string {
+ return userId.split(':').pop() || 'unknown';
+}
+
</file context>
| * @example extractOriginFromMatrixRoomId('!room:matrix.org') // 'matrix.org' | ||
| */ | ||
| export function extractOriginFromMatrixRoomId(roomId: string): string { | ||
| return roomId.split(':').pop() || 'unknown'; |
There was a problem hiding this comment.
P2: Origin extraction uses the substring after the last colon. Matrix IDs should be split at the first colon per spec, so IDs containing additional colons (e.g., server name with port) will produce incorrect origins. Consider extracting everything after the first colon instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/federation-sdk/src/metrics/helpers.ts, line 6:
<comment>Origin extraction uses the substring after the last colon. Matrix IDs should be split at the first colon per spec, so IDs containing additional colons (e.g., server name with port) will produce incorrect origins. Consider extracting everything after the first colon instead.</comment>
<file context>
@@ -0,0 +1,87 @@
+ * @example extractOriginFromMatrixRoomId('!room:matrix.org') // 'matrix.org'
+ */
+export function extractOriginFromMatrixRoomId(roomId: string): string {
+ return roomId.split(':').pop() || 'unknown';
+}
+
</file context>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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/metrics/index.ts">
<violation number="1" location="packages/federation-sdk/src/metrics/index.ts:40">
P2: getOrCreateMetric only checks the default registry, so after switching to a custom registry the getters can register duplicate metrics on every access. Consider looking up metrics in the same registry you register them with.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| name: 'rocketchat_federation_events_processed', | ||
| labelNames: ['event_type', 'direction'], | ||
| help: 'Total federation events processed', | ||
| registers: [registry], |
There was a problem hiding this comment.
P2: getOrCreateMetric only checks the default registry, so after switching to a custom registry the getters can register duplicate metrics on every access. Consider looking up metrics in the same registry you register them with.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/federation-sdk/src/metrics/index.ts, line 40:
<comment>getOrCreateMetric only checks the default registry, so after switching to a custom registry the getters can register duplicate metrics on every access. Consider looking up metrics in the same registry you register them with.</comment>
<file context>
@@ -31,6 +37,7 @@ export const federationMetrics = {
name: 'rocketchat_federation_events_processed',
labelNames: ['event_type', 'direction'],
help: 'Total federation events processed',
+ registers: [registry],
}),
);
</file context>
3 participants
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.