Fix: constructLogEntry drops extra open-record fields (e.g. error) — resolves wso2/product-integrator#152#562
Open
SasinduDilshara wants to merge 5 commits intowso2:mainfrom
Open
Fix: constructLogEntry drops extra open-record fields (e.g. error) — resolves wso2/product-integrator#152#562SasinduDilshara wants to merge 5 commits intowso2:mainfrom
SasinduDilshara wants to merge 5 commits intowso2:mainfrom
Conversation
Fixes wso2/product-integrator#152 — ICP log viewer was silently dropping any field not in the hardcoded list inside constructLogEntry(). The LogSource type is an open record, so fields like 'error' (emitted by Ballerina runtimes as complex JSON objects) were present after deserialization but never written into the logfmt output string. The fix iterates all non-handled entries in the open record and appends them as key=value pairs, skipping empty/nil values. A companion unit-test file is added to cover the bug, the regression, and edge cases. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Author
Issue Analysis — [Issue #152]: ICP log viewer is not showing error messagesClassification
Reproducibility
Root Cause AnalysisThe bug is entirely in
However,
Any field not in this list is silently ignored. The The constructed string is stored as Fix required: The relevant change is in // After building the known fields, append any remaining fields from the open record
// (e.g. 'error', custom application fields)
string extraFields = "";
// Known fields that are already handled explicitly
string[] knownFields = ["time", "level", "message", "service_type", "module", "app_name",
"artifact_container", "traceId", "spanId", "icp_runtimeId",
"@timestamp", "log_file_path", "product", "deployment", "app",
"app_module", "class", "icp.runtimeId"];
map<anydata> sourceMap = <map<anydata>>sourceData;
foreach [string, anydata] [key, val] in sourceMap.entries() {
if knownFields.indexOf(key) is () && val != () && val.toString() != "" {
extraFields += string ` ${key}=${val.toString()}`;
}
}
return string `time=${time} level=${level}${serviceSpecificFields} message="${message}"${traceId}${spanId}${runtimeId}${extraFields}`;Test Coverage Assessment
|
Contributor
WalkthroughThe changes enhance the log entry construction function to capture and include additional fields from OpenSearch documents that were previously ignored, alongside adding comprehensive test coverage to validate the new behavior with complex error objects. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
Comment on lines
+773
to
+776
| string[] knownFields = ["time", "level", "message", "service_type", "module", "app_name", | ||
| "artifact_container", "traceId", "spanId", "icp_runtimeId", | ||
| "@timestamp", "log_file_path", "product", "deployment", "app", | ||
| "app_module", "class", "icp.runtimeId"]; |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
Suggested change
| string[] knownFields = ["time", "level", "message", "service_type", "module", "app_name", | |
| "artifact_container", "traceId", "spanId", "icp_runtimeId", | |
| "@timestamp", "log_file_path", "product", "deployment", "app", | |
| "app_module", "class", "icp.runtimeId"]; | |
| string[] knownFields = ["time", "level", "message", "service_type", "module", "app_name", | |
| "artifact_container", "traceId", "spanId", "icp_runtimeId", | |
| "@timestamp", "log_file_path", "product", "deployment", "app", | |
| "app_module", "class", "icp.runtimeId"]; | |
| log.debug("Processing extra fields for log entry construction"); |
Comment on lines
+777
to
+778
| string extraFields = ""; | ||
| map<anydata> sourceMap = <map<anydata>>sourceData; |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
Suggested change
| string extraFields = ""; | |
| map<anydata> sourceMap = <map<anydata>>sourceData; | |
| string extraFields = ""; | |
| map<anydata> sourceMap = <map<anydata>>sourceData; | |
| if (log.isDebugEnabled()) { | |
| log.debug(string `Checking for extra fields in source map with ${sourceMap.length()} total fields`); | |
| } |
Contributor
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
| Comment | Accepted (Y/N) | Reason |
|---|---|---|
| #### Log Improvement Suggestion No: 1 | ||
| #### Log Improvement Suggestion No: 2 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
icp_server/tests/log_entry_construction_test.bal (2)
32-38: Use a real nested JSON object for the complex-error fixture.The current fixture stores
erroras a pre-serialized string. Using an actual nested object better matches the OpenSearch complex-field scenario.💡 Proposed fixture refinement
- "error": "{\"causes\":[{\"message\":\"Connection refused: localhost/127.0.0.1:9445\",\"detail\":{},\"stackTrace\":[]}],\"message\":\"Something wrong with the connection\",\"detail\":{},\"stackTrace\":[]}", + "error": { + "causes": [ + { + "message": "Connection refused: localhost/127.0.0.1:9445", + "detail": {}, + "stackTrace": [] + } + ], + "message": "Something wrong with the connection", + "detail": {}, + "stackTrace": [] + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@icp_server/tests/log_entry_construction_test.bal` around lines 32 - 38, The test fixture in log_entry_construction_test uses sourceJson with the "error" field set to a serialized string; update the fixture so sourceJson's "error" property is a real nested JSON object (e.g., an object with "causes", "message", "detail", "stackTrace") instead of a JSON string so the test mirrors the OpenSearch complex-field scenario; locate the sourceJson variable in log_entry_construction_test.bal and replace the string value for "error" with an actual JSON object structure.
108-110: Align extra-field assertions with logfmt-safe formatting.This assertion currently hardcodes an unquoted spaced value. If extra-field serialization is made logfmt-safe, this expectation should be updated to quoted output.
💡 Proposed test update
- test:assertTrue(logEntry.includes("error=Connection refused"), "Log entry must include 'error' extra field"); + test:assertTrue(logEntry.includes("error=\"Connection refused\""), "Log entry must include quoted 'error' extra field");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@icp_server/tests/log_entry_construction_test.bal` around lines 108 - 110, The assertions in log_entry_construction_test.bal currently expect unquoted extra-field values (e.g., test:assertTrue(logEntry.includes("error=Connection refused"))), which will fail if extra-field serialization is made logfmt-safe; update the three test:assertTrue assertions that reference logEntry to expect quoted values instead (check for error="Connection refused", errorCode="ERR_CONN_REFUSED", and httpStatus="503") so the assertions match logfmt-safe output while still using the existing logEntry variable and same test:assertTrue calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@icp_server/opensearch_adapter_service.bal`:
- Around line 779-784: The loop that builds extraFields from sourceMap (foreach
[key, val] in sourceMap.entries()) appends raw strVal which can contain spaces
or quotes and will break logfmt; update the code where extraFields is appended
(currently using string ` ${key}=${strVal}`) to first escape backslashes and
quotes in strVal and wrap the value in double quotes (e.g., transform " -> \"
and \ -> \\), or call a new helper like escapeForLogfmt(strVal) that performs
this escaping, then append using the quoted-escaped value so extraFields and
logfmt tokenization remain correct.
---
Nitpick comments:
In `@icp_server/tests/log_entry_construction_test.bal`:
- Around line 32-38: The test fixture in log_entry_construction_test uses
sourceJson with the "error" field set to a serialized string; update the fixture
so sourceJson's "error" property is a real nested JSON object (e.g., an object
with "causes", "message", "detail", "stackTrace") instead of a JSON string so
the test mirrors the OpenSearch complex-field scenario; locate the sourceJson
variable in log_entry_construction_test.bal and replace the string value for
"error" with an actual JSON object structure.
- Around line 108-110: The assertions in log_entry_construction_test.bal
currently expect unquoted extra-field values (e.g.,
test:assertTrue(logEntry.includes("error=Connection refused"))), which will fail
if extra-field serialization is made logfmt-safe; update the three
test:assertTrue assertions that reference logEntry to expect quoted values
instead (check for error="Connection refused", errorCode="ERR_CONN_REFUSED", and
httpStatus="503") so the assertions match logfmt-safe output while still using
the existing logEntry variable and same test:assertTrue calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d435576-aeec-423b-8ba2-ea4948a88443
📒 Files selected for processing (2)
icp_server/opensearch_adapter_service.balicp_server/tests/log_entry_construction_test.bal
Comment on lines
+779
to
+784
| foreach [string, anydata] [key, val] in sourceMap.entries() { | ||
| if knownFields.indexOf(key) is () { | ||
| string strVal = val.toString(); | ||
| if strVal != "" && strVal != "()" { | ||
| extraFields += string ` ${key}=${strVal}`; | ||
| } |
Contributor
There was a problem hiding this comment.
Quote and escape extra-field values before appending to logfmt.
At Line 783, raw strVal is appended directly. Values with spaces/quotes (for example error messages) can break logfmt tokenization and truncate/split field values.
💡 Proposed fix
foreach [string, anydata] [key, val] in sourceMap.entries() {
if knownFields.indexOf(key) is () {
string strVal = val.toString();
if strVal != "" && strVal != "()" {
- extraFields += string ` ${key}=${strVal}`;
+ boolean requiresQuotes = strVal.contains(" ") || strVal.contains("=") || strVal.contains("\"");
+ string escapedVal = strVal.replace("\\", "\\\\").replace("\"", "\\\"");
+ string formattedVal = requiresQuotes ? string `"${escapedVal}"` : escapedVal;
+ extraFields += string ` ${key}=${formattedVal}`;
}
}
}📝 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.
Suggested change
| foreach [string, anydata] [key, val] in sourceMap.entries() { | |
| if knownFields.indexOf(key) is () { | |
| string strVal = val.toString(); | |
| if strVal != "" && strVal != "()" { | |
| extraFields += string ` ${key}=${strVal}`; | |
| } | |
| foreach [string, anydata] [key, val] in sourceMap.entries() { | |
| if knownFields.indexOf(key) is () { | |
| string strVal = val.toString(); | |
| if strVal != "" && strVal != "()" { | |
| boolean requiresQuotes = strVal.contains(" ") || strVal.contains("=") || strVal.contains("\""); | |
| string escapedVal = strVal.replace("\\", "\\\\").replace("\"", "\\\""); | |
| string formattedVal = requiresQuotes ? string `"${escapedVal}"` : escapedVal; | |
| extraFields += string ` ${key}=${formattedVal}`; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@icp_server/opensearch_adapter_service.bal` around lines 779 - 784, The loop
that builds extraFields from sourceMap (foreach [key, val] in
sourceMap.entries()) appends raw strVal which can contain spaces or quotes and
will break logfmt; update the code where extraFields is appended (currently
using string ` ${key}=${strVal}`) to first escape backslashes and quotes in
strVal and wrap the value in double quotes (e.g., transform " -> \" and \ ->
\\), or call a new helper like escapeForLogfmt(strVal) that performs this
escaping, then append using the quoted-escaped value so extraFields and logfmt
tokenization remain correct.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
constructLogEntry()inicp_server/opensearch_adapter_service.balonly assembled a hardcoded set of known fields into the logfmt output string. Any extra field present in the openLogSourcerecord (e.g.error,errorCode, custom app fields emitted by Ballerina runtimes) was silently ignored, so the ICP log viewer never displayed it.key=valuepairs, skipping empty/nil values.icp_server/tests/log_entry_construction_test.balcovers: the bug scenario (complexerrorfield), basic regression, multiple extra fields, and empty-field skipping.Changes
icp_server/opensearch_adapter_service.balconstructLogEntry()icp_server/tests/log_entry_construction_test.balconstructLogEntry()Linked issue
Fixes wso2/product-integrator#152 — ICP log viewer is not showing error messages.
Summary by CodeRabbit
Release Notes
New Features
Tests