Publish translation logs to moesif from translator #28
Conversation
WalkthroughThis PR integrates Moesif analytics into the FTP-SFTP translator module. Changes include documentation for Moesif setup, new configuration blocks and types for analytics settings, dependency version update, and refactored logging to publish structured JSON events to both dashboard and Moesif endpoints. Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as handleError/handleSkip/handleSuccess
participant Event as createPublishableTranslationEvent
participant Dashboard as Dashboard Logger
participant Moesif as sendToMoesif
participant Client as Moesif HTTP Client
Handler->>Event: Build structured event<br/>(messages, IDs, metadata, status)
Event-->>Handler: Return JSON event
Handler->>Dashboard: appendToDashboardLogs(event)
Dashboard->>Dashboard: Log JSON event
Handler->>Moesif: sendToMoesif(event)
Moesif->>Moesif: Transform to Moesif format
Moesif->>Client: POST to Moesif API
Client-->>Moesif: Response
Moesif->>Moesif: Handle errors asynchronously
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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
🧹 Nitpick comments (3)
README.md (1)
116-133: Consider clarifying the Moesif and OpenSearch relationship.Line 132 states "display real-time analytics from Moesif instead of OpenSearch," but based on
utils.bal, dashboard logs are still written locally viaappendToDashboardLogs(which writes to files that OpenSearch can index). Moesif appears to be an additional analytics channel rather than a replacement.Consider rewording to: "This enables additional real-time analytics through Moesif alongside existing dashboard logging."
translator/integrations/ftp-sftp/swiftMtMxTranslator/utils.bal (2)
446-452: Inconsistent case sensitivity in error categorization.The error message checks have inconsistent case handling:
"required"check is case-sensitive (line 446, 450)"not supported"check is case-sensitive on line 447 but case-insensitive on line 448"invalid"check is case-insensitive (line 448-449)This could lead to errors being miscategorized. Consider normalizing the error message once:
+ string lowerErrorMsg = errorMsg.toLowerAscii(); return { // ... other fields ... - "fieldError": errorMsg.includes("required") ? errorMsg : "", - "notSupportedError": errorMsg.toLowerAscii().includes("not supported") ? errorMsg : "", - "invalidError": errorMsg.toLowerAscii().includes("invalid") - && !errorMsg.toLowerAscii().includes("not supported") ? errorMsg : "", - "otherError": !errorMsg.includes("required") && !errorMsg.includes("not supported") - && !errorMsg.toLowerAscii().includes("invalid") ? errorMsg : "" + "fieldError": lowerErrorMsg.includes("required") ? errorMsg : "", + "notSupportedError": lowerErrorMsg.includes("not supported") ? errorMsg : "", + "invalidError": lowerErrorMsg.includes("invalid") + && !lowerErrorMsg.includes("not supported") ? errorMsg : "", + "otherError": !lowerErrorMsg.includes("required") && !lowerErrorMsg.includes("not supported") + && !lowerErrorMsg.includes("invalid") ? errorMsg : "" };
471-484: Unused variablestatusValuein worker block.The
statusValuevariable is extracted (lines 473-475) but never used in the Moesif action payload or elsewhere. This appears to be dead code that should be removed or utilized.worker MoesifWorker { do { - // Extract status for action name - json statusField = check publishableTranslationEvent.status; - string statusValue = statusField is string ? statusField : ""; - // Transform to Moesif action format json moesifAction = { "action_name": "translation_log",Alternatively, if the intention was to include status in the action name:
- "action_name": "translation_log", + "action_name": string `translation_${statusValue}`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.md(1 hunks)translator/integrations/ftp-sftp/README.md(1 hunks)translator/integrations/ftp-sftp/swiftMtMxTranslator/Config.toml(2 hunks)translator/integrations/ftp-sftp/swiftMtMxTranslator/Dependencies.toml(1 hunks)translator/integrations/ftp-sftp/swiftMtMxTranslator/configurables.bal(1 hunks)translator/integrations/ftp-sftp/swiftMtMxTranslator/types.bal(1 hunks)translator/integrations/ftp-sftp/swiftMtMxTranslator/utils.bal(5 hunks)
🔇 Additional comments (6)
translator/integrations/ftp-sftp/README.md (1)
213-235: Documentation for Moesif Analytics is clear and complete.The optional configuration section is well-documented with all required fields, default values, and setup steps. The TOML snippet aligns with the
MoesifConfigtype definition.translator/integrations/ftp-sftp/swiftMtMxTranslator/Dependencies.toml (1)
418-441: Version bump looks appropriate for the new Moesif integration.The package version update to 2.0.5 aligns with the new feature addition.
translator/integrations/ftp-sftp/swiftMtMxTranslator/configurables.bal (1)
115-125: Moesif configurable block is well-structured with sensible defaults.
enabled: falseprevents accidental activation- Retry configuration with exponential backoff is appropriate for external API calls
- Default timeout of 5 seconds is reasonable for analytics calls
translator/integrations/ftp-sftp/swiftMtMxTranslator/Config.toml (1)
117-131: Moesif configuration block is well-documented and safely disabled by default.The placeholder
applicationIdandenabled = falseensure users must explicitly configure and enable the feature. Comments provide clear guidance.translator/integrations/ftp-sftp/swiftMtMxTranslator/utils.bal (1)
46-58: Handler updates follow a consistent pattern for event publishing.The refactored
handleError,handleSkip, andhandleSuccessfunctions consistently create publishable events and send them to both the dashboard (synchronous) and Moesif (asynchronous). This maintains backward compatibility while adding the new analytics capability.translator/integrations/ftp-sftp/swiftMtMxTranslator/types.bal (1)
57-66: Type inconsistency forretryBackOffFactoris valid — but context-dependent on Ballerina http module version.The concern about using
floatforretryBackOffFactorwhile other timeout/interval fields usedecimalis legitimate:
- Ballerina does NOT support implicit type coercion between
floatanddecimal— they are distinct IEEE 754 types (binary64 vs decimal128 subset).- http:RetryConfig.backOffFactor type varies by version:
- Older http module:
backOffFactorisfloat- Newer Ballerina specs:
backOffFactorisdecimal- If using a newer Ballerina version,
http:RetryConfig.backOffFactorexpectsdecimal, requiring explicit conversion of thefloatvalue — creating unnecessary type friction.The recommendation to use
decimalconsistently aligns with current Ballerina best practices and newer http module specifications. However, verify the exact http module version inBallerina.tomlto confirm whether this change is necessary or merely a consistency improvement.type MoesifConfig record { boolean enabled = false; string applicationId; string apiEndpoint = "https://api.moesif.net/v1/actions"; decimal timeout = 5.0; int retryCount = 3; decimal retryInterval = 2.0; - float retryBackOffFactor = 2.0; + decimal retryBackOffFactor = 2.0; decimal retryMaxWaitInterval = 10.0; };
| // Initialize Moesif HTTP client (only if enabled) | ||
| final http:Client? moesifClient = moesif.enabled ? check new (moesif.apiEndpoint, { | ||
| timeout: moesif.timeout, | ||
| retryConfig: { | ||
| count: moesif.retryCount, | ||
| interval: moesif.retryInterval, | ||
| backOffFactor: moesif.retryBackOffFactor, | ||
| maxWaitInterval: moesif.retryMaxWaitInterval | ||
| } | ||
| }) : (); |
There was a problem hiding this comment.
Module-level check can crash the application on startup for a non-essential feature.
Using check in the module-level client initialization means any error (invalid apiEndpoint URL format, etc.) will cause the entire application to fail at startup. Analytics should fail gracefully without affecting core translation functionality.
Consider using error handling to make Moesif initialization failures non-fatal:
-final http:Client? moesifClient = moesif.enabled ? check new (moesif.apiEndpoint, {
- timeout: moesif.timeout,
- retryConfig: {
- count: moesif.retryCount,
- interval: moesif.retryInterval,
- backOffFactor: moesif.retryBackOffFactor,
- maxWaitInterval: moesif.retryMaxWaitInterval
- }
-}) : ();
+final http:Client? moesifClient = initMoesifClient();
+
+function initMoesifClient() returns http:Client? {
+ if !moesif.enabled {
+ return ();
+ }
+ http:Client|error client = new (moesif.apiEndpoint, {
+ timeout: moesif.timeout,
+ retryConfig: {
+ count: moesif.retryCount,
+ interval: moesif.retryInterval,
+ backOffFactor: moesif.retryBackOffFactor,
+ maxWaitInterval: moesif.retryMaxWaitInterval
+ }
+ });
+ if client is error {
+ log:printError("Failed to initialize Moesif client. Analytics will be disabled.", err = client.toBalString());
+ return ();
+ }
+ return client;
+}📝 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.
| // Initialize Moesif HTTP client (only if enabled) | |
| final http:Client? moesifClient = moesif.enabled ? check new (moesif.apiEndpoint, { | |
| timeout: moesif.timeout, | |
| retryConfig: { | |
| count: moesif.retryCount, | |
| interval: moesif.retryInterval, | |
| backOffFactor: moesif.retryBackOffFactor, | |
| maxWaitInterval: moesif.retryMaxWaitInterval | |
| } | |
| }) : (); | |
| // Initialize Moesif HTTP client (only if enabled) | |
| final http:Client? moesifClient = initMoesifClient(); | |
| function initMoesifClient() returns http:Client? { | |
| if !moesif.enabled { | |
| return (); | |
| } | |
| http:Client|error client = new (moesif.apiEndpoint, { | |
| timeout: moesif.timeout, | |
| retryConfig: { | |
| count: moesif.retryCount, | |
| interval: moesif.retryInterval, | |
| backOffFactor: moesif.retryBackOffFactor, | |
| maxWaitInterval: moesif.retryMaxWaitInterval | |
| } | |
| }); | |
| if client is error { | |
| log:printError("Failed to initialize Moesif client. Analytics will be disabled.", err = client.toBalString()); | |
| return (); | |
| } | |
| return client; | |
| } |
| // Transform to Moesif action format | ||
| json moesifAction = { | ||
| "action_name": "translation_log", | ||
| "request": { | ||
| "time": check publishableTranslationEvent.date | ||
| }, | ||
| "metadata": publishableTranslationEvent | ||
| }; |
There was a problem hiding this comment.
Consider privacy implications of sending full message content to Moesif.
The publishableTranslationEvent containing originalMessage and translatedMessage (which include full SWIFT MT/MX message content with financial transaction details) is sent to Moesif's external servers.
Consider:
- Documenting this data flow in the README (users should be aware PII/transaction data goes to a third-party)
- Adding an option to exclude message content from Moesif payloads:
// Option to send only metadata without message content
json moesifMetadata = moesif.excludeMessageContent ?
publishableTranslationEvent.clone() but { "originalMessage": "[redacted]", "translatedMessage": "[redacted]" } :
publishableTranslationEvent;This is especially relevant for SWIFT messages which may contain sensitive financial and PII data subject to regulations.
🤖 Prompt for AI Agents
In translator/integrations/ftp-sftp/swiftMtMxTranslator/utils.bal around lines
477 to 484, the code sends publishableTranslationEvent (including
originalMessage and translatedMessage) to Moesif which may leak sensitive
SWIFT/financial/PII data; add a configurable option (e.g.,
moesif.excludeMessageContent) and when enabled create a sanitized copy of
publishableTranslationEvent that replaces or removes originalMessage and
translatedMessage (e.g., set to "[redacted]" or omit the keys) and use that
sanitized object for moesifAction.metadata; additionally update the README to
document that message content is sent to Moesif and how to enable the
excludeMessageContent setting.
1 participant
Purpose
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.