chore: NATS broker for micro services - #35305
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 |
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #35305 +/- ##
===========================================
+ Coverage 69.33% 69.36% +0.03%
===========================================
Files 4255 4259 +4
Lines 168644 168954 +310
Branches 30052 30144 +92
===========================================
+ Hits 116929 117200 +271
- Misses 46534 46567 +33
- Partials 5181 5187 +6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@kody start-review |
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
![kody-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
| const serviceEvents = new Set<{ | ||
| eventName: keyof EventSignatures; | ||
| listeners: { | ||
| (...args: any[]): void; | ||
| }[]; | ||
| }>(); |
There was a problem hiding this comment.
private serviceEvents = new Set<{
eventName: keyof EventSignatures;
listeners: {
(...args: any[]): void;
}[];
}>();Potential memory leaks due to global serviceEvents Set
This issue appears in multiple locations:
- ee/packages/network-broker/src/NatsBroker.ts: Lines 12-17
Please move serviceEvents to the class instance and implement cleanup in destroyService
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| import { MoleculerBroker } from './MoleculerBroker'; | ||
|
|
||
| const { |
There was a problem hiding this comment.
function validateEnvConfig(config: Record<string, string>) {
const numberFields = ['REQUEST_TIMEOUT', 'HEARTBEAT_INTERVAL', 'HEARTBEAT_TIMEOUT', 'RETRY_FACTOR', 'RETRY_MAX_DELAY'];
const booleanFields = ['RETRY_ENABLED', 'BULKHEAD_ENABLED', 'MS_METRICS'];
for (const field of numberFields) {
if (isNaN(parseInt(config[field]))) {
throw new Error(`Invalid ${field} value: must be a number`);
}
}
for (const field of booleanFields) {
if (![undefined, 'true', 'false'].includes(config[field])) {
throw new Error(`Invalid ${field} value: must be 'true' or 'false'`);
}
}
return config;
}
const config = validateEnvConfig(process.env);Missing function description comments
This issue appears in multiple locations:
- ee/packages/network-broker/src/moleculer.ts: Lines 9-32
- ee/packages/network-broker/src/getInstanceMethods.ts: Lines 3-3
Please add detailed function description comments for better code understanding
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } catch (e) { | ||
| console.error('error', e); | ||
| } |
There was a problem hiding this comment.
} catch (e) {
const error = e instanceof Error ? e : new Error('Unknown error');
console.error('Service method execution failed:', error);
if (msg?.respond) {
msg.respond(TE.encode(EJSON.stringify({ error: error.message })));
}
}Insufficient error handling in service methods
This issue appears in multiple locations:
- ee/packages/network-broker/src/NatsBroker.ts: Lines 87-89
- ee/packages/network-broker/src/NatsBroker.ts: Lines 121-123
Please improve error handling in service methods with proper error propagation
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| }; | ||
|
|
||
| export class NetworkBroker implements IBroker { | ||
| export class MoleculerBroker implements IBroker { |
There was a problem hiding this comment.
/**
* Implementation of IBroker using Moleculer service broker
* for handling service communication and event distribution.
*/
export class MoleculerBroker implements IBroker {Inadequate type definitions for error handling
This issue appears in multiple locations:
- ee/packages/network-broker/src/MoleculerBroker.ts: Lines 21-21
- ee/packages/network-broker/src/moleculer.ts: Lines 37-37
Please add proper type definitions for error handling to improve type safety
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
3b2cc8e to
6d69c4a
Compare
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
6d69c4a to
7b81a9c
Compare
f48f4b5 to
05ab9ff
Compare
🔴 Layne — 2 finding(s)Found 2 issue(s): 1 high, 1 medium. |
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot <copilot@github.com>
20065d4 to
b8c6d68
Compare
EnterpriseCheck was a Moleculer ServiceSchema, so the policy it implements (no scalability module means only one service instance per environment) was unavailable to any other broker. Move the policy into licenseEnforcement and have the mixin delegate to it, so NatsBroker can reuse it. The failure counter is now per service instead of a module level counter shared by every mixin instance, which is what MAX_FAILS consecutive failures was meant to mean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Events and service methods shared one flat subject space, so a broadcast of `accounts.login` invoked the `accounts` service `login` method and a call to that method was delivered to the event listeners. Methods now answer on `rpc.<service>.<method>` and events publish on `event.<name>`. The node id is published as micro service metadata, which is what the rest of this change is built on: - `nodeList()` is served from `$SRV.PING` discovery instead of returning an empty array, and `$node.services` is answered from the same source so apps-engine keeps working - each method also answers on `node.<nodeID>.<service>.<method>`, so `CallingOptions.nodeID` reaches one instance rather than being load balanced across the queue group - non internal services get the shared license enforcement, so selecting this broker no longer turns the scalability check off Also in the same pass, because they touch the same registration path: - requests use REQUEST_TIMEOUT rather than the nats default of one second - `destroyService` stops the micro service, drains the event subscriptions and removes the instance listeners; events are tracked per instance instead of in a module level Set that was never pruned - one subscription per event routed through `emit`, so listeners registered after startup are still reached - NATS_URL replaces the overloaded TRANSPORTER, which compose passes through as an empty string and so bypassed the old destructuring default Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A throwing endpoint only logged, never answered, so the caller blocked until the request timeout and then saw a timeout instead of the real error. Reply with the nats micro error headers plus an EJSON body, and rehydrate it on the calling side. MeteorError is serialized through toJSON and restored with its error code, reason, details and isClientSafe intact, which is what the Moleculer CustomRegenerator does for the other broker. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was no coverage for NatsBroker at all. Add an in memory stand in for NatsConnection covering the parts the broker uses - subject routing, request/reply, the micro registry and discovery - and specs for the subject namespaces, node scoped routing, error round trip, discovery and teardown. The namespace specs are the regression guard: broadcasting `accounts.login` must not invoke the `accounts` service `login` method, and calling that method must not notify the event listeners. Also renames the MoleculerBroker describe block, still labelled NetworkBroker after the rename. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The lockfile resolved two distinct nats versions, 2.28.2 for the app and the microservices and 2.29.1 for network-broker, so two copies of the client were installed. Pin everything to ^2.29.1. The dev services env file had no way to select the broker, so local microservices development could only run on moleculer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The e2e workflow forced BROKER=nats for every EE suite, replacing the Moleculer coverage rather than adding to it. Take the broker as an input defaulting to moleculer, so callers opt in per suite. The readiness gate accepted either broker's startup log, so a silent fallback would have passed unnoticed; it now waits for the broker that was asked for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
https://rocketchat.atlassian.net/browse/RDI-278
Proposed changes (including videos or screenshots)
This is currently a PoC. The idea is to provide a broker for our services architecture that relies only on NATS, so we make use of all its features.
These are tracked items that are still missing:
Issue(s)
Steps to test or reproduce
Further comments
This pull request introduces significant changes to the Rocket.Chat repository, specifically targeting the integration of a NATS broker for microservices. The key modifications include:
Renaming and Refactoring:
NetworkBrokerhas been renamed toMoleculerBrokerin theMoleculerBroker.tsfile. Additionally, the method discovery logic has been extracted into a newgetInstanceMethodsfunction to enhance code organization.NATS Broker Implementation:
NatsBroker.tsfile has been added, implementing a NATS-based broker service. This service includes capabilities for event handling, service creation, and message broadcasting.Test Updates:
MoleculerBroker.test.tshas been updated to replaceNetworkBrokerwithMoleculerBroker, ensuring that the test functionality for service destruction remains intact.Code Organization:
index.tsfile has been simplified by moving broker implementation details to separate files. It now exportsMoleculerBroker,NatsBroker, and thestartBrokerfunction.Moleculer Configuration:
moleculer.tsfile introduces a configuration for the Moleculer service broker, including custom error handling, serialization, and various settings managed through environment variables.API Class Update:
Apiclass inApi.tshas been updated to include a newstartedproperty, along with related changes to manage the broker's start method.These changes aim to enhance the microservices architecture within the Rocket.Chat platform by integrating a robust NATS broker and improving the existing broker infrastructure.