feat: Implement detailed tracing #38167
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
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. WalkthroughSwitch tracing from a proxy-based approach to decorator-based tracing, add a tracer middleware factory and addSpanAttributes API, instrument models and federation code with decorators/attributes, remove legacy traceInstanceMethods, and add an observability stack (Tempo/OTLP/Prometheus/Grafana) plus test-script support and configs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant MW as tracerSpanMiddleware()
participant App as App Handler
participant Decor as traced/tracedClass
participant OT as OpenTelemetry
Client->>MW: HTTP request
MW->>OT: Start span "METHOD <url>" with attrs (url, method, userId?)
MW->>App: await next()
App->>Decor: Invoke decorated methods
Decor->>OT: Start method-level span and add attributes
Decor-->>App: Return result
App-->>MW: Handler completes
MW->>OT: Record response status
MW->>Client: Set X-Trace-Id (if span) and return response
sequenceDiagram
participant Caller as Caller
participant Wrapper as tracedClass wrapper
participant Method as Original method
participant OT as OpenTelemetry
Caller->>Wrapper: Call Class.method(...)
Wrapper->>OT: Start span "<type> Class.method" + base attrs
Wrapper->>Method: Execute original method
Method-->>Wrapper: Return value
Wrapper->>OT: End span
Wrapper-->>Caller: Return value
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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: 3
🤖 Fix all issues with AI agents
In @ee/packages/federation-matrix/tests/scripts/run-integration-tests.sh:
- Line 410: The log message has a typo in the test command string "yarn
testend-to-end"; update the string passed to log_info (the line that logs manual
run instructions) to use the correct test script name, e.g., "yarn
test:end-to-end" (or the correct project-specific task such as "yarn
test:federation" if that is the intended script), so the logged instruction is
accurate.
In @packages/tracing/src/middlewares/tracerSpanMiddleware.ts:
- Around line 21-25: The current code attempts to read userId from
c.req.raw.clone() which returns a Request and never has a runtime userId;
replace that logic to read the value from the Hono context instead (use
c.get('userId') and/or c.get('user')?.id as appropriate) and then set
attributes.userId when that value is present; remove the casted clone usage
(c.req.raw.clone()) and update the tracerSpanMiddleware code that sets
attributes.userId to pull from c.get(...) so the runtime value is actually read
from the Hono context.
In @packages/tracing/src/tracedDecorator.ts:
- Around line 49-54: Update the JSDoc in tracedDecorator.ts for the
getTraceExtractor (or equivalent) function so the decorator reference uses the
correct lowercase form `@traced` instead of `@Traced`; edit the comment text to
replace `@Traced` with `@traced` to match the actual decorator name.
🧹 Nitpick comments (4)
ee/packages/federation-matrix/docker-compose/observability/collector.config.yml (1)
11-24: Unusedloggingexporter in pipeline configuration.The
loggingexporter is defined at lines 12-13 but is not included in the traces pipeline exporters (line 24). If debug logging of traces is desired during development, add it to the exporters list.🔧 Proposed fix to enable trace logging
service: pipelines: traces: receivers: [otlp] processors: [batch] - exporters: [otlp/1] + exporters: [logging, otlp/1]packages/tracing/src/traceInstanceMethods.ts (2)
67-90: Type safety concern with span attributes.The
attributesobject can containparameters: unknown[](line 85) when no extractor is found, but it's later cast toRecord<string, string | number | boolean | undefined>(line 90). OpenTelemetry span attributes support arrays of primitives (AttributeValue), but this cast may cause unexpected serialization behavior for complex parameter objects.Consider either:
- Serializing the parameters array to a JSON string for tracing
- Using the correct OpenTelemetry attribute types (
SpanAttributeValue)Option 1: Serialize parameters to JSON string
} else { // Fallback to raw parameters for methods without extractors - attributes.parameters = getArguments(argumentsList); + attributes.parameters = JSON.stringify(getArguments(argumentsList)); }
76-82: Silent catch may hide extractor bugs during development.The empty catch block silently swallows all extractor errors. While this is appropriate for production robustness (tracing shouldn't break functionality), consider logging at debug level to aid troubleshooting.
Optional: Add debug logging
try { const extractedAttrs = extractor(...(argumentsList as unknown[])); Object.assign(attributes, extractedAttrs); -} catch { +} catch (e) { // If extractor fails, continue with base attributes + if (process.env.DEBUG_TRACING) { + console.debug(`Trace extractor failed for ${className}.${prop}:`, e); + } }packages/tracing/src/middlewares/tracerSpanMiddleware.ts (1)
27-28: Consider using route pattern instead of full URL for span names to avoid high cardinality.Using
c.req.urldirectly in the span name will create unique spans for URLs with dynamic segments (e.g.,/users/123,/rooms/abc-def), leading to high cardinality in your tracing backend.💡 Potential improvement
return tracerSpan( - `${c.req.method} ${c.req.url}`, + `${c.req.method} ${c.req.path}`,Even better would be using the matched route pattern (e.g.,
/users/:id) if available from the Hono router context afternext()resolves. For now, usingc.req.pathinstead of full URL at least removes query strings and origin.
📜 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)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (23)
apps/meteor/app/api/server/api.tsapps/meteor/app/api/server/middlewares/tracer.tsapps/meteor/ee/server/apps/communication/rest.tsee/packages/federation-matrix/README.mdee/packages/federation-matrix/docker-compose.test.ymlee/packages/federation-matrix/docker-compose/observability/agent.ymlee/packages/federation-matrix/docker-compose/observability/collector.config.ymlee/packages/federation-matrix/docker-compose/observability/grafana-datasources.ymlee/packages/federation-matrix/docker-compose/observability/prometheus.ymlee/packages/federation-matrix/docker-compose/observability/tempo.ymlee/packages/federation-matrix/package.jsonee/packages/federation-matrix/src/FederationMatrix.tsee/packages/federation-matrix/src/api/_matrix/transactions.tsee/packages/federation-matrix/src/api/routes.tsee/packages/federation-matrix/src/tracing.tsee/packages/federation-matrix/tests/scripts/run-integration-tests.shpackages/models/src/models/BaseRaw.tspackages/tracing/package.jsonpackages/tracing/src/index.tspackages/tracing/src/middlewares/tracerSpanMiddleware.tspackages/tracing/src/traceInstanceMethods.tspackages/tracing/src/tracedDecorator.tspackages/tsconfig/base.json
💤 Files with no reviewable changes (1)
- apps/meteor/app/api/server/middlewares/tracer.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
ee/packages/federation-matrix/src/tracing.tspackages/tracing/src/index.tspackages/tracing/src/middlewares/tracerSpanMiddleware.tsapps/meteor/ee/server/apps/communication/rest.tspackages/tracing/src/tracedDecorator.tsee/packages/federation-matrix/src/api/routes.tspackages/models/src/models/BaseRaw.tsapps/meteor/app/api/server/api.tspackages/tracing/src/traceInstanceMethods.tsee/packages/federation-matrix/src/api/_matrix/transactions.tsee/packages/federation-matrix/src/FederationMatrix.ts
🧠 Learnings (11)
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
ee/packages/federation-matrix/package.jsonee/packages/federation-matrix/src/FederationMatrix.ts
📚 Learning: 2025-11-05T21:04:35.787Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
Applied to files:
ee/packages/federation-matrix/package.jsonee/packages/federation-matrix/src/api/routes.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
ee/packages/federation-matrix/package.json
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
ee/packages/federation-matrix/package.json
📚 Learning: 2025-12-09T20:01:00.324Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:00.324Z
Learning: When reviewing federation invite handling in Rocket.Chat (specifically under ee/packages/federation-matrix), understand that rejecting an invite via federationSDK.rejectInvite() triggers an event-driven cleanup: a leave event is emitted and handled by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, which calls Room.performUserRemoval() to remove the subscription. Do not add explicit cleanup in the reject branch of handleInvite(); rely on the existing leave-event flow for cleanup. If making changes, ensure this invariant remains and that any related paths still funnel cleanup through the leave event to avoid duplicate or missing removals.
Applied to files:
ee/packages/federation-matrix/src/tracing.tsee/packages/federation-matrix/src/api/routes.tsee/packages/federation-matrix/src/api/_matrix/transactions.tsee/packages/federation-matrix/src/FederationMatrix.ts
📚 Learning: 2025-09-15T13:10:30.049Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 36868
File: packages/http-router/src/Router.ts:416-425
Timestamp: 2025-09-15T13:10:30.049Z
Learning: In packages/http-router/src/Router.ts, the dispatch() method's use of replaceAll('//', '/') on the full path is acceptable because URL normalization and query string handling is performed by the caller function before dispatch() is invoked.
Applied to files:
ee/packages/federation-matrix/src/api/routes.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
ee/packages/federation-matrix/src/api/_matrix/transactions.tsee/packages/federation-matrix/src/FederationMatrix.ts
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.ts
📚 Learning: 2025-10-01T12:20:13.147Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37110
File: apps/meteor/ee/server/startup/federation.ts:17-21
Timestamp: 2025-10-01T12:20:13.147Z
Learning: In Rocket.Chat's StreamerCentral broadcast for 'notify-room' events with 'user-activity', the `activity` argument is a string (e.g., 'user-typing') when a user starts typing and an empty array when the user stops typing.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.ts
🧬 Code graph analysis (9)
ee/packages/federation-matrix/src/tracing.ts (2)
packages/tracing/src/traceInstanceMethods.ts (1)
ITraceInstanceMethodsOptions(16-26)packages/core-typings/src/IRoom.ts (1)
IRoomNativeFederated(113-120)
packages/tracing/src/index.ts (1)
apps/meteor/client/lib/e2ee/logger.ts (1)
span(35-37)
packages/tracing/src/middlewares/tracerSpanMiddleware.ts (1)
packages/tracing/src/index.ts (1)
tracerSpan(40-91)
apps/meteor/ee/server/apps/communication/rest.ts (1)
packages/tracing/src/middlewares/tracerSpanMiddleware.ts (1)
tracerSpanMiddleware(14-43)
ee/packages/federation-matrix/src/api/routes.ts (1)
packages/tracing/src/middlewares/tracerSpanMiddleware.ts (1)
tracerSpanMiddleware(14-43)
packages/models/src/models/BaseRaw.ts (1)
packages/tracing/src/traceInstanceMethods.ts (1)
traceInstanceMethods(53-102)
apps/meteor/app/api/server/api.ts (1)
packages/tracing/src/middlewares/tracerSpanMiddleware.ts (1)
tracerSpanMiddleware(14-43)
packages/tracing/src/traceInstanceMethods.ts (2)
packages/tracing/src/tracedDecorator.ts (1)
getTraceExtractor(55-60)packages/tracing/src/index.ts (1)
tracerActiveSpan(93-106)
ee/packages/federation-matrix/src/FederationMatrix.ts (7)
packages/tracing/src/traceInstanceMethods.ts (1)
traceInstanceMethods(53-102)packages/tracing/src/tracedDecorator.ts (1)
traced(39-47)packages/core-typings/src/IRoom.ts (3)
IRoom(20-94)IRoomNativeFederated(113-120)isRoomNativeFederated(124-125)packages/core-typings/src/IUser.ts (2)
IUser(187-258)isUserNativeFederated(279-280)packages/tracing/src/index.ts (1)
addSpanAttributes(132-137)ee/packages/federation-matrix/src/events/message.ts (1)
message(113-412)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(145-242)
🪛 Biome (2.1.2)
packages/models/src/models/BaseRaw.ts
[error] 86-86: The constructor should not return a value.
The constructor is here:
Returning a value from a constructor may confuse users of the class.
(lint/correctness/noConstructorReturn)
ee/packages/federation-matrix/src/FederationMatrix.ts
[error] 151-151: The constructor should not return a value.
The constructor is here:
Returning a value from a constructor may confuse users of the class.
(lint/correctness/noConstructorReturn)
🪛 markdownlint-cli2 (0.18.1)
ee/packages/federation-matrix/README.md
88-88: Bare URL used
(MD034, no-bare-urls)
89-89: Bare URL used
(MD034, no-bare-urls)
91-91: Bare URL used
(MD034, no-bare-urls)
97-97: Bare URL used
(MD034, no-bare-urls)
98-98: Bare URL used
(MD034, no-bare-urls)
99-99: Bare URL used
(MD034, no-bare-urls)
⏰ 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). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🔇 Additional comments (42)
packages/tracing/package.json (1)
20-21: No action needed. Hono is properly used in the package source code (packages/tracing/src/middlewares/tracerSpanMiddleware.tsimportsMiddlewareHandlerfrom Hono for the tracing middleware implementation). The version^4.10.7is valid and secure (latest version is 4.11.4, which is compatible with the constraint).Likely an incorrect or invalid review comment.
ee/packages/federation-matrix/docker-compose/observability/prometheus.yml (1)
1-14: LGTM!The Prometheus configuration is well-structured with reasonable defaults for a test/dev observability stack. The 15-second scrape interval is appropriate for this use case, and the scrape jobs correctly target the expected services in the docker-compose network.
packages/tsconfig/base.json (1)
7-8: LGTM!Enabling
experimentalDecoratorsis required for the@traceddecorator used in the FederationMatrix service. This change correctly supports the new tracing infrastructure.Note: If the decorators rely on runtime type information (e.g., for parameter injection), you may also need
emitDecoratorMetadata: true. Verify this if you encounter issues with decorator metadata.ee/packages/federation-matrix/docker-compose/observability/grafana-datasources.yml (1)
1-31: LGTM!The Grafana datasources configuration is well-structured. Setting Tempo as the default datasource is appropriate for a tracing-focused observability stack, and the service map integration with Prometheus enables useful trace-to-metrics correlation.
apps/meteor/ee/server/apps/communication/rest.ts (2)
11-11: LGTM!The import correctly consolidates the tracing middleware from the centralized
@rocket.chat/tracingpackage, aligning with the PR's goal of unified distributed tracing infrastructure.
74-77: LGTM!The middleware chain is correctly configured with proper invocation of
tracerSpanMiddleware()as a factory function. The ordering (logger → metrics → tracer) is sensible, allowing traces to capture the full request lifecycle including logging and metrics operations.ee/packages/federation-matrix/README.md (2)
28-28: Clear and comprehensive documentation for the new flag.The
--observabilityflag documentation thoroughly explains its purpose, automatic behaviors (includes Element, keeps containers running), and intended use case for manual verification of tracing/metrics collection.
93-109: Well-structured observability services section.The documentation clearly lists all observability endpoints, explains the automatic configuration (OTLP, Prometheus scraping, Grafana datasources), and appropriately sets expectations that verification is manual.
ee/packages/federation-matrix/docker-compose/observability/tempo.yml (2)
13-26: All receivers enabled - appropriate for test environment.The configuration enables all receiver protocols which is suitable for the integration test environment. The inline comment appropriately warns that production deployments should minimize enabled receivers.
28-29: Demo-specific ingester setting noted.The
max_block_duration: 5msetting with the inline comment clarifies this is for demo purposes. This is appropriate for the test environment where faster block rotation helps with observability during short test runs.ee/packages/federation-matrix/tests/scripts/run-integration-tests.sh (3)
54-59: Clean flag implementation with sensible defaults.The
--observabilityflag automatically enablesKEEP_RUNNINGandINCLUDE_ELEMENT, which makes sense since users need running containers to inspect traces/metrics manually.
188-191: Good defensive cleanup for yarn config restoration.The cleanup function properly restores
.yarnrc.ymlfrom backup if it exists, handling early exit scenarios gracefully.
220-241: Multi-platform yarn configuration for Docker compatibility.The backup/configure/restore pattern for yarn's
supportedArchitecturesensures correct binaries (sharp, etc.) are available for the Alpine container while preserving the user's original config.packages/tracing/src/traceInstanceMethods.ts (1)
13-26: Well-designed options interface.The
ITraceInstanceMethodsOptionsinterface provides a clean, extensible API. The requiredtypefield ensures consistent span naming, and optionalignoreMethodsmaintains backward compatibility.packages/models/src/models/BaseRaw.ts (1)
86-86: Correct migration to new traceInstanceMethods API.The addition of
{ type: 'model' }aligns with the new options-based signature. This will produce span names likemodel BaseRaw.findOnefor better trace categorization.Regarding the static analysis warning about returning from a constructor: this pattern (returning a Proxy from constructor) is pre-existing and intentional for transparent method interception. The proxy maintains the same interface as
this, so consumers are unaffected.ee/packages/federation-matrix/src/api/routes.ts (1)
2-2: LGTM!The tracing middleware is correctly imported and positioned in the router chain after authorization checks (
isFederationEnabledMiddleware,isLicenseEnabledMiddleware) but before route handlers. This ensures traces are only created for authorized requests, which is the appropriate ordering.Also applies to: 26-26
ee/packages/federation-matrix/package.json (1)
31-31: LGTM!The
@rocket.chat/tracingdependency is correctly added using the workspace protocol, consistent with other internal package dependencies in this file.ee/packages/federation-matrix/docker-compose/observability/agent.yml (1)
1-16: LGTM!The observability agent configuration is appropriate for the test environment:
log_level: debugandinsecure: trueare acceptable for local docker-compose testing infrastructure.- Batch settings (5s timeout, 100 batch size) are reasonable defaults for development tracing.
apps/meteor/app/api/server/api.ts (1)
4-4: LGTM!The tracing middleware is correctly migrated to use the external
@rocket.chat/tracingpackage. The factory function pattern (tracerSpanMiddleware()) aligns with the package's export signature and is consistently applied across both the main API and federation routes.Also applies to: 109-109
ee/packages/federation-matrix/src/api/_matrix/transactions.ts (1)
339-359: LGTM!The span attribute enrichment is well-designed for federation trace analysis:
- Follows OpenTelemetry semantic conventions with the
federation.*namespace.- Boolean flags for common PDU/EDU types (
has_message,has_membership, etc.) enable efficient trace filtering.- Comma-joined type lists provide flexibility for discovering less common event types.
- The code safely handles missing
pdus/edusarrays with defaults, and the schema validation (isSendTransactionBodyProps) ensures the body structure is valid before this code executes.packages/tracing/src/index.ts (2)
13-14: LGTM!The new exports cleanly expose the
tracedDecoratorandtracerSpanMiddlewaremodules, making them available through the package's public API for use in other packages like federation-matrix.
113-137: LGTM!The
addSpanAttributesfunction is well-designed with:
- Clear JSDoc documentation with a practical example
- Graceful no-op behavior when no active span exists
- Flexible
Record<string, any>type for diverse attribute valuesThis enables enriching spans with runtime-discovered data without requiring callers to handle the absence of an active span.
packages/tracing/src/middlewares/tracerSpanMiddleware.ts (1)
32-40: LGTM!Good practices observed:
- Setting
X-Trace-Idheader enables request correlation for debugging- Recording response status after
next()captures the actual outcome- Optional chaining on
span?.setAttributehandles disabled tracing gracefullyee/packages/federation-matrix/docker-compose.test.yml (5)
5-8: LGTM!New network and volume for the observability stack are properly defined.
243-261: LGTM!The init container pattern correctly addresses Tempo's permission requirements. Running as root to chown the volume before Tempo starts (as user 10001) is the standard approach.
263-296: LGTM!The Tempo and otel-collector configuration is well-structured:
- YAML anchor reuse for the Tempo image version
- Proper dependency on the init container
- Standard ports for OTLP ingestion (4317 gRPC, 4318 HTTP)
328-343: Anonymous admin access is acceptable for test environment only.The Grafana configuration disables authentication (
GF_AUTH_ANONYMOUS_ENABLED=true,GF_AUTH_DISABLE_LOGIN_FORM=true) with admin role. This is appropriate for a local test/dev environment but should never be used in production.
123-127: LGTM!Tracing environment variables are consistently applied to both local and prebuilt RC configurations. The OTEL exporter endpoint correctly points to the collector service name within the observability network.
Also applies to: 169-172
ee/packages/federation-matrix/src/tracing.ts (4)
1-8: LGTM!Clean imports and well-documented module purpose. The
ITraceInstanceMethodsOptions['attributeExtractors']type ensures type safety for the extractor map.
10-43: LGTM!Room and message operation extractors are well-structured:
- Consistent attribute naming across extractors
- Safe optional chaining for all property accesses
- Boolean normalization for flags (hasFiles, hasThread, hasAttachments)
- Extracts both Rocket.Chat IDs and Matrix IDs where available
64-122: LGTM!Invitation and membership extractors correctly capture the relevant context:
- Invite operations include invitee counts and inviter details
- Leave/kick operations track the kicker when present
- The
hasKicker: Boolean(kicker)pattern is good for nullable fields
124-189: LGTM!Settings, role, typing, verification, and user management extractors are consistent with the established patterns. Count-based attributes (
matrixIdCount,usernameCount) are useful for understanding operation scope in traces.ee/packages/federation-matrix/src/FederationMatrix.ts (6)
216-242: LGTM!The
@traceddecorator oncreateRoomcaptures the initial context (room/owner details), andaddSpanAttributesenriches with the Matrix response data. This two-phase approach effectively captures both input and output context.
284-363: LGTM!The
createDirectMessageRoomtracing is comprehensive:
- Initial decorator captures room, member count, and creator
- Runtime attributes add computed values (isGroupDM, matrixUserId)
- Conditional attributes for 1:1 vs group DM paths
- Final matrixRoomId captured after creation
493-538: LGTM!The
sendMessagetracing effectively captures:
- Message metadata via decorator (hasFiles, hasThread, hasAttachments)
- Computed values (matrixUserId, messageType, isNativeFederatedUser)
- Result data (matrixEventId)
The separation between decorator and runtime attributes is appropriate.
647-700: LGTM!The
sendReactionmethod demonstrates good practice of adding runtime attributes after database lookups resolve (room, message federation data), providing complete context in the span.
769-806: LGTM!Good use of
addSpanAttributesto record skip reasons (skipped: true, reason: '...'). This provides valuable debugging context when operations are short-circuited.
1063-1114: LGTM!The
handleInvitetracing correctly captures the action type (accept/reject) and enriches with runtime-resolved user/room data. The error handling for 403 responses follows the established pattern per the learnings about federation invite cleanup.packages/tracing/src/tracedDecorator.ts (4)
1-4: LGTM!Using a Symbol as a unique property key is the correct approach for storing metadata on methods without risking property name collisions.
6-16: LGTM!Type definitions are well-structured with appropriate generics. The optional property on
TracedMethodcorrectly reflects that not all methods will have an extractor attached.
39-47: LGTM!The decorator factory correctly implements the method decorator pattern. Attaching the extractor to the original method via Symbol key is clean and allows
traceInstanceMethodsto retrieve it later.
55-60: LGTM!The type guard and defensive check for
typeof method === 'function'ensures safe property access. Clean implementation.
ee/packages/federation-matrix/tests/scripts/run-integration-tests.sh
Outdated
Show resolved
Hide resolved
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 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="ee/packages/federation-matrix/src/api/_matrix/transactions.ts">
<violation number="1" location="ee/packages/federation-matrix/src/api/_matrix/transactions.ts:343">
P2: EDUs may lack the `edu_type` property, causing `undefined` values in trace attributes. Filter out undefined values when mapping to avoid polluting observability data.</violation>
</file>
<file name="ee/packages/federation-matrix/docker-compose.test.yml">
<violation number="1" location="ee/packages/federation-matrix/docker-compose.test.yml:312">
P2: Pin Prometheus to a specific version instead of using 'latest' tag for reproducible test environments.</violation>
</file>
<file name="packages/tracing/src/middlewares/tracerSpanMiddleware.ts">
<violation number="1" location="packages/tracing/src/middlewares/tracerSpanMiddleware.ts:22">
P2: The userId extraction logic is broken. Cloning a Request object and type-asserting it to have a `userId` property won't work - `Request.clone()` only copies standard Request properties, not custom properties. This code will never capture userId in trace attributes.
If userId is attached to the request context by upstream middleware, access it directly from the Hono context (e.g., `c.get('userId')`) or from the original request object without cloning.</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/tracing/src/tracingDecorators.ts`:
- Around line 63-121: The wrapper currently records full call arguments (via
attributes.parameters and getArguments) when no extractor exists; change this to
record only a sanitized summary unless a method explicitly opts in via `@traced`:
update getArguments (or replace its usage in traceInstanceMethods) so it returns
a small summary object like { count: argumentsList.length, types:
['String','Object',...], hasSession: boolean } or similar type/count info, and
set attributes.parameters to that summary; keep the existing behavior of using
getTraceExtractor(...) when present, and ensure no raw argument values are ever
attached to span attributes unless a method has opted-in via the `@traced`
extractor metadata.
🧹 Nitpick comments (2)
packages/tracing/src/index.ts (1)
131-135: Type-narrowaddSpanAttributesto OpenTelemetryAttributes.
Record<string, any>allows invalid attribute values; using the library’s attribute type gives compile-time safety and aligns withsetAttributesexpectations.♻️ Proposed type tightening
-import type { Span, SpanOptions, Tracer } from '@opentelemetry/api'; +import type { Attributes, Span, SpanOptions, Tracer } from '@opentelemetry/api'; ... -export function addSpanAttributes(attributes: Record<string, any>): void { +export function addSpanAttributes(attributes: Attributes): void { const span = trace.getActiveSpan(); if (span) { span.setAttributes(attributes); } }ee/packages/federation-matrix/src/FederationMatrix.ts (1)
234-240: Remove inline “Add runtime attributes” comments.These comments are redundant with the code and add noise; please drop them (and similar ones below) to keep the implementation clean. As per coding guidelines, please avoid implementation comments.
🧹 Example cleanup
- // Add runtime attributes after Matrix room is created addSpanAttributes({ matrixRoomId: matrixRoomResult.room_id, matrixEventId: matrixRoomResult.event_id, matrixUserId, visibility: room.t === 'c' ? 'public' : 'invite', });Also applies to: 300-305, 507-512
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)
ee/packages/federation-matrix/tests/scripts/run-integration-tests.sh (1)
388-411: Update manual test instruction to match the actual command executed.
The script runsyarn test:federationat line 393, but the manual instruction at line 407 directs users to runyarn test:end-to-end. Update the instruction to reference the correct script:Fix
- log_info "To run tests manually, execute: yarn test:end-to-end" + log_info "To run tests manually, execute: yarn test:federation"
🤖 Fix all issues with AI agents
In `@ee/packages/federation-matrix/docker-compose.test.yml`:
- Around line 263-275: The tempo service currently publishes an ephemeral host
port via the ports entry ('ports: ['3200']'), so advertised endpoint
http://localhost:3200 may not be reachable; update the tempo service's ports
configuration (for service name "tempo" and its existing ports entry) to
explicitly bind host port 3200 to container port 3200 (i.e., replace the
ephemeral mapping with an explicit host:container mapping) and keep the other
port mappings unchanged.
- Around line 298-305: The docker-compose service "agent" is using the
deprecated grafana/agent:v0.27.1 and Agent YAML; update it to Grafana Alloy by
replacing the image reference (grafana/agent:v0.27.1) with the official Alloy
image (e.g., grafana/alloy:<desired-version>), change the mounted config file
from ./docker-compose/observability/agent.yml to an Alloy config (e.g.,
./docker-compose/observability/agent.alloy), and update the entrypoint/args from
"/bin/agent -config.file=/etc/agent.yaml" to the Alloy binary and config flag
per the migration guide (use the Alloy executable and its config flag such as
--config.file=/etc/agent.alloy); also migrate the agent.yml contents into the
Alloy .alloy format following the Grafana Alloy migration guide to ensure
compatibility.
| agent: | ||
| image: grafana/agent:v0.27.1 | ||
| volumes: | ||
| - ./docker-compose/observability/agent.yml:/etc/agent.yaml | ||
| entrypoint: | ||
| - /bin/agent | ||
| - -config.file=/etc/agent.yaml | ||
| profiles: |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Grafana Agent deprecated replaced Grafana Alloy migration
💡 Result:
Short answer: Grafana Agent (Static / Flow / Operator) is deprecated and being replaced by Grafana Alloy (Grafana’s OpenTelemetry Collector distribution). Agent entered LTS in April 2024 and will reach End‑of‑Life on November 1, 2025 — you should migrate to Alloy. [1][2]
What to do (high level)
- Choose Alloy as the replacement (same overall collector functionality, Alloy config uses .alloy files / OTEL components). [1][3]
- Follow the official migration guide that matches your Agent variant:
- Flow → Alloy (live migration steps, copy data dir, update pipelines/configs). [3]
- Agent Operator → Alloy (Helm values, CRD differences, supported Monitor types). [4]
- Static / other variants → see respective migrate pages in Grafana docs. [2][5]
- Test in staging, copy Agent data directories to Alloy data directory if doing an in-place migration, and update clients to send to Alloy receivers before disabling Agent to avoid data loss/duplication. [3][4]
Links / docs (official)
- Grafana blog announcement and LTS/EOL timeline. [1]
- Alloy “one year” + EOL reminder. [2]
- Migrate Flow → Alloy docs. [3]
- Migrate Agent Operator → Alloy docs. [4]
If you want, tell me which Agent variant and your deployment type (Kubernetes, Docker, or host) and I’ll give step‑by‑step commands and a checklist.
Migrate from Grafana Agent to Grafana Alloy immediately.
Grafana Agent reached End-of-Life on November 1, 2025 and is no longer supported. Upgrade to Grafana Alloy (the official replacement) following the migration guide for your deployment type. Update the image reference and configuration format from Agent YAML to Alloy's .alloy format.
🤖 Prompt for AI Agents
In `@ee/packages/federation-matrix/docker-compose.test.yml` around lines 298 -
305, The docker-compose service "agent" is using the deprecated
grafana/agent:v0.27.1 and Agent YAML; update it to Grafana Alloy by replacing
the image reference (grafana/agent:v0.27.1) with the official Alloy image (e.g.,
grafana/alloy:<desired-version>), change the mounted config file from
./docker-compose/observability/agent.yml to an Alloy config (e.g.,
./docker-compose/observability/agent.alloy), and update the entrypoint/args from
"/bin/agent -config.file=/etc/agent.yaml" to the Alloy binary and config flag
per the migration guide (use the Alloy executable and its config flag such as
--config.file=/etc/agent.alloy); also migrate the agent.yml contents into the
Alloy .alloy format following the Grafana Alloy migration guide to ensure
compatibility.
There was a problem hiding this comment.
@ggazzo what are we using in production?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
dhulke
force-pushed
the
feat/federation-tracing2
branch
from
c8b999c to
e1759a6
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38167 +/- ##
========================================
Coverage 70.68% 70.68%
========================================
Files 3136 3136
Lines 108592 108592
Branches 19570 19577 +7
========================================
+ Hits 76756 76757 +1
- Misses 29827 29833 +6
+ Partials 2009 2002 -7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 Docker Image Size Report➡️ Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "01/18 22:05 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.00]
line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Sun, 18 Jan 2026 22:05:01 GMT |
…nd enhance cross-platform build configuration in integration tests
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.