carverauto/serviceradar

Fork 0

Vendor/k8s ocaml #2299

Merged

mfreeman451 merged 9 commits from refs/pull/2299/head into main

2025-10-07 03:30:38 +00:00

mfreeman451 commented

2025-10-07 02:53:47 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #1728
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1728
Original created: 2025-10-07T02:53:47Z
Original updated: 2025-10-07T03:30:43Z
Original head: carverauto/serviceradar:vendor/k8s_ocaml
Original base: main
Original merged: 2025-10-07T03:30:38Z by @mfreeman451

PR Type

Enhancement, Bug fix

Description

Add device identity canonicalization to prevent duplicate device IDs
Implement backfill CLI for historical data migration
Optimize logging to reduce noise and improve performance
Add BuildBuddy chart and OCaml stdlib vendoring

Diagram Walkthrough

flowchart LR
  A["Device Updates"] --> B["Registry Canonicalization"]
  B --> C["Identity Maps"]
  C --> D["Canonical Device ID"]
  D --> E["Database Storage"]
  F["Backfill CLI"] --> G["Historical Data Migration"]
  G --> E
  H["Logging Optimization"] --> I["Reduced Log Noise"]

File Walkthrough

Relevant files

Enhancement

9 files

main.go `Add backfill CLI flags and execution logic`	+79/-41
flush.go `Add record size limits and truncation logic`	+68/-26
metrics.go `Optimize sweep processing logging`	+18/-32
pollers.go `Replace verbose service logging with summaries`	+30/-41
services.go `Reduce debug logging noise in service processing`	+28/-58
discovery.go `Implement batch processing for discovered interfaces`	+91/-24
utils.go `Change alternate IP storage to exact-match keys`	+4/-23
registry.go `Implement device identity canonicalization system`	+510/-5
sweeper.go `Change device target scanning to primary IP only`	+16/-35

Bug fix

4 files

discovery.go `Fix JSON decoder and alternate IP metadata handling`	+19/-18
result_processor.go `Fix logging field name consistency`	+8/-8
pollers.go `Remove details field from service status queries`	+9/-9
unified_devices.go `Remove alternate IP lookup from device queries`	+5/-9

Tests

5 files

performance_integration_test.go `Add mock expectations for canonicalization queries`	+23/-8
batch_optimization_test.go `Add canonicalization query mocks to tests`	+3/-0
registry_test.go `Add canonicalization query mocks to registry tests`	+10/-0
retraction_processing_test.go `Add canonicalization query mocks to retraction tests`	+4/-0
aggregation_test.go `Update tests for primary IP only scanning`	+83/-126

Documentation

4 files

services.go `Add comments for legacy message column`	+46/-46
ImplementationPlan.md `Add KV-backed canonical identity map implementation plan`	+92/-0
architecture.md `Document device identity canonicalization and backfill`	+29/-1
README.md `Add OCaml stdlib vendoring documentation`	+22/-0

Configuration changes

1 files

values.yaml
Add BuildBuddy Helm chart configuration +60/-0

Imported from GitHub pull request. Original GitHub pull request: #1728 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/1728 Original created: 2025-10-07T02:53:47Z Original updated: 2025-10-07T03:30:43Z Original head: carverauto/serviceradar:vendor/k8s_ocaml Original base: main Original merged: 2025-10-07T03:30:38Z by @mfreeman451 --- ### **PR Type** Enhancement, Bug fix ___ ### **Description** - Add device identity canonicalization to prevent duplicate device IDs - Implement backfill CLI for historical data migration - Optimize logging to reduce noise and improve performance - Add BuildBuddy chart and OCaml stdlib vendoring ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Device Updates"] --> B["Registry Canonicalization"] B --> C["Identity Maps"] C --> D["Canonical Device ID"] D --> E["Database Storage"] F["Backfill CLI"] --> G["Historical Data Migration"] G --> E H["Logging Optimization"] --> I["Reduced Log Noise"] ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><details><summary>9 files</summary><table> <tr> <td><strong>main.go</strong><dd><code>Add backfill CLI flags and execution logic</code>                              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1728/files#diff-4ab3fd1d4debc53dd2499d94a0f60c648fdae4235dd1e3678095a975f5bb434a">+79/-41</a>  </td> </tr> <tr> <td><strong>flush.go</strong><dd><code>Add record size limits and truncation logic</code>                            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1728/files#diff-4ad1cf4b0397bd277cd9c0553e4992ad7c9e9484d49d49d48af015958dc4bf4b">+68/-26</a>  </td> </tr> <tr> <td><strong>metrics.go</strong><dd><code>Optimize sweep processing logging</code>                                                </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1728/files#diff-6d98e853ce17576c088e77956ae4ecfa8078019e0bff107a79d8d1d6ed2443ad">+18/-32</a>  </td> </tr> <tr> <td><strong>pollers.go</strong><dd><code>Replace verbose service logging with summaries</code>                      </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1728/files#diff-fe81e2a32f1ac64bcdc6f25f55c5fa918d17bad8c0546f2cf80c757ff4051816">+30/-41</a>  </td> </tr> <tr> <td><strong>services.go</strong><dd><code>Reduce debug logging noise in service processing</code>                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1728/files#diff-b75091f9768dcdaf46aedeee40cb2eaa33b46a484d77d5d432bab19fe437237f">+28/-58</a>  </td> </tr> <tr> <td><strong>discovery.go</strong><dd><code>Implement batch processing for discovered interfaces</code>          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1728/files#diff-29a4ce8613152068c4493f784ec41cf340e445b6b6e7c5ddb698826f9a4f1341">+91/-24</a>  </td> </tr> <tr> <td><strong>utils.go</strong><dd><code>Change alternate IP storage to exact-match keys</code>                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1728/files#diff-23330adb94a3b6d90f70f8b08d876097eff0288bf8e4be8e44fdc5e9e1a60166">+4/-23</a>    </td> </tr> <tr> <td><strong>registry.go</strong><dd><code>Implement device identity canonicalization system</code>                </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1728/files#diff-cb61d8f79451b9541de4a8cc0811523a68d15452b2f5971c7618ea5b423cf4ec">+510/-5</a>  </td> </tr> <tr> <td><strong>sweeper.go</strong><dd><code>Change device target scanning to primary IP only</code>                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1728/files#diff-d11dee1504de681453c5a04e22ae44d64820221ac6b84a1630d3a3929dace47a">+16/-35</a>  </td> </tr> </table></details></td></tr><tr><td><strong>Bug fix</strong></td><td><details><summary>4 files</summary><table> <tr> <td><strong>discovery.go</strong><dd><code>Fix JSON decoder and alternate IP metadata handling</code>            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1728/files#diff-037880b0f5f345614f813cca12ec429765f07396878b6f177b4259eb3a4f5f0e">+19/-18</a>  </td> </tr> <tr> <td><strong>result_processor.go</strong><dd><code>Fix logging field name consistency</code>                                              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1728/files#diff-828f49c36998b59e7eea7ceca2e1982ce06366bca483c9a52439a360ce2a0a50">+8/-8</a>      </td> </tr> <tr> <td><strong>pollers.go</strong><dd><code>Remove details field from service status queries</code>                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1728/files#diff-4e4ff6d32240a5f2e8d053438abcc4a77959d521bc99028ed2bbcf1e07145631">+9/-9</a>      </td> </tr> <tr> <td><strong>unified_devices.go</strong><dd><code>Remove alternate IP lookup from device queries</code>                      </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1728/files#diff-165bffede411b9a031b4d85af0ae459a1913960593d59bfc19554cb838e39679">+5/-9</a>      </td> </tr> </table></details></td></tr><tr><td><strong>Tests</strong></td><td><details><summary>5 files</summary><table> <tr> <td><strong>performance_integration_test.go</strong><dd><code>Add mock expectations for canonicalization queries</code>              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1728/files#diff-d770ddfc6c616fd6c54cb34b5405ef9a2d9f7b56dbd34d39c4b2ea2eeb066599">+23/-8</a>    </td> </tr> <tr> <td><strong>batch_optimization_test.go</strong><dd><code>Add canonicalization query mocks to tests</code>                                </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1728/files#diff-cac8ebc53144bf9cd9a61b9ed7c11907afc9e3b57574885e6c440276295a8314">+3/-0</a>      </td> </tr> <tr> <td><strong>registry_test.go</strong><dd><code>Add canonicalization query mocks to registry tests</code>              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1728/files#diff-f010972d104404be52d2a8e6e784cb56e31194f90795a69571a12696bcbdc075">+10/-0</a>    </td> </tr> <tr> <td><strong>retraction_processing_test.go</strong><dd><code>Add canonicalization query mocks to retraction tests</code>          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1728/files#diff-ada575691eeb382afc69b92133daac730cedca73481463ba41f81676bf745b22">+4/-0</a>      </td> </tr> <tr> <td><strong>aggregation_test.go</strong><dd><code>Update tests for primary IP only scanning</code>                                </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1728/files#diff-0c6eb392da3429542428a3c3242866632ba312a670e5face8b1d295fca100d82">+83/-126</a></td> </tr> </table></details></td></tr><tr><td><strong>Documentation</strong></td><td><details><summary>4 files</summary><table> <tr> <td><strong>services.go</strong><dd><code>Add comments for legacy message column</code>                                      </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1728/files#diff-08bcc70ff62d6bd79ef5139eb844e853b9b999a2ebde0e75f8e546afb44ff022">+46/-46</a>  </td> </tr> <tr> <td><strong>ImplementationPlan.md</strong><dd><code>Add KV-backed canonical identity map implementation plan</code>  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1728/files#diff-5da5f582062c3e5eb14e5b07da0130e44ae49bb2b03a56115a874f031076c0b6">+92/-0</a>    </td> </tr> <tr> <td><strong>architecture.md</strong><dd><code>Document device identity canonicalization and backfill</code>      </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1728/files#diff-90abd06467420fd89391fd1a4d75ceb1f6a9381de4d13a95fffe606abff38d37">+29/-1</a>    </td> </tr> <tr> <td><strong>README.md</strong><dd><code>Add OCaml stdlib vendoring documentation</code>                                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1728/files#diff-6bb0f490f3f528e61a401c040689dd04b4dc1a429227fbc6d4c2afa97f38f39a">+22/-0</a>    </td> </tr> </table></details></td></tr><tr><td><strong>Configuration changes</strong></td><td><details><summary>1 files</summary><table> <tr> <td><strong>values.yaml</strong><dd><code>Add BuildBuddy Helm chart configuration</code>                                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1728/files#diff-0e6a8eb2ccb1111460bf4dc2fe0433f6ffd30b93d586924e8413ac0a2f3cc9c1">+60/-0</a>    </td> </tr> </table></details></td></tr></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-10-07 02:55:00 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1728#issuecomment-3374979757
Original created: 2025-10-07T02:55:00Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Identity resolution change Description: Relaxing IP lookup to exclude alternate IP metadata may break existing identity resolution paths and allow stale or orphaned device records to be missed, potentially affecting authorization or visibility; verify that removing metadata-based IP matching is intentional and won’t cause data exposure through mis-association. unified_devices.go [241-248] Referred Code conditions = append(conditions, fmt.Sprintf("ip IN (%s)", strings.Join(ipList, ","))) // Note: We intentionally do not include metadata alt_ip:* matches here // to avoid conflating historical/alternate IPs with primary identity. } query := fmt.Sprintf(`SELECT
	Unsafe JSON escaping Description: Manual string escaping in JSON summary (`escapeForJSON`) risks malformed JSON or injection if edge cases are missed; safer to build an object and marshal via json.Marshal instead of hand-escaping. flush.go [289-316] Referred Code // truncateStatusDetailsIfTooLarge ensures a single service status record stays under the shard limit func (Server) truncateStatusDetailsIfTooLarge(status models.ServiceStatus, maxBytes int) { if status == nil \|\| len(status.Details) <= maxBytes { return } // Prepare a compact summary JSON with sizes and a short preview orig := status.Details previewLen := 256 if len(orig) < previewLen { previewLen = len(orig) } summary := fmt.Sprintf(`{"truncated":true,"original_size":%d,"stored_size":%d,"preview":"%s"}`, len(orig), previewLen, escapeForJSON(string(orig[:previewLen])), ) status.Details = json.RawMessage(summary) } // escapeForJSON escapes backslashes and quotes for embedding in JSON func escapeForJSON(s string) string { ... (clipped 7 lines)
	Secret in config Description: BuildBuddy executor config includes an empty api_key field which, if populated insecurely via values, could expose credentials in plain text; ensure secrets are sourced from Kubernetes Secrets and not committed in values.yaml. values.yaml [53-60] Referred Code `config: executor: app_target: grpcs://remote.buildbuddy.io:443 enable_oci: true local_cache_directory: /cache local_cache_size_bytes: 50000000000 root_directory: /cache/remotebuilds/ api_key: ""`
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
⚪	No custom compliance provided Follow the guide to enable custom compliance check.
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1728#issuecomment-3374979757 Original created: 2025-10-07T02:55:00Z --- ## PR Compliance Guide 🔍  Below is a summary of compliance checks for this PR:<br> <table><tbody><tr><td colspan='2'><strong>Security Compliance</strong></td></tr> <tr><td rowspan=3>⚪</td> <td><details><summary><strong>Identity resolution change </strong></summary><br> <b>Description:</b> Relaxing IP lookup to exclude alternate IP metadata may break existing identity resolution <br>paths and allow stale or orphaned device records to be missed, potentially affecting <br>authorization or visibility; verify that removing metadata-based IP matching is <br>intentional and won’t cause data exposure through mis-association.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1728/files#diff-165bffede411b9a031b4d85af0ae459a1913960593d59bfc19554cb838e39679R241-R248'>unified_devices.go [241-248]</a></strong><br> <details open><summary>Referred Code</summary> ```go conditions = append(conditions, fmt.Sprintf("ip IN (%s)", strings.Join(ipList, ","))) // Note: We intentionally do not include metadata alt_ip:* matches here // to avoid conflating historical/alternate IPs with primary identity. } query := fmt.Sprintf(`SELECT ``` </details></details></td></tr> <tr><td><details><summary><strong>Unsafe JSON escaping </strong></summary><br> <b>Description:</b> Manual string escaping in JSON summary (<code>escapeForJSON</code>) risks malformed JSON or injection <br>if edge cases are missed; safer to build an object and marshal via json.Marshal instead of <br>hand-escaping.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1728/files#diff-4ad1cf4b0397bd277cd9c0553e4992ad7c9e9484d49d49d48af015958dc4bf4bR289-R316'>flush.go [289-316]</a></strong><br> <details open><summary>Referred Code</summary> ```go // truncateStatusDetailsIfTooLarge ensures a single service status record stays under the shard limit func (*Server) truncateStatusDetailsIfTooLarge(status *models.ServiceStatus, maxBytes int) { if status == nil || len(status.Details) <= maxBytes { return } // Prepare a compact summary JSON with sizes and a short preview orig := status.Details previewLen := 256 if len(orig) < previewLen { previewLen = len(orig) } summary := fmt.Sprintf(`{"truncated":true,"original_size":%d,"stored_size":%d,"preview":"%s"}`, len(orig), previewLen, escapeForJSON(string(orig[:previewLen])), ) status.Details = json.RawMessage(summary) } // escapeForJSON escapes backslashes and quotes for embedding in JSON func escapeForJSON(s string) string { ... (clipped 7 lines) ``` </details></details></td></tr> <tr><td><details><summary><strong>Secret in config </strong></summary><br> <b>Description:</b> BuildBuddy executor config includes an empty api_key field which, if populated insecurely <br>via values, could expose credentials in plain text; ensure secrets are sourced from <br>Kubernetes Secrets and not committed in values.yaml.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1728/files#diff-0e6a8eb2ccb1111460bf4dc2fe0433f6ffd30b93d586924e8413ac0a2f3cc9c1R53-R60'>values.yaml [53-60]</a></strong><br> <details open><summary>Referred Code</summary> ```yaml config: executor: app_target: grpcs://remote.buildbuddy.io:443 enable_oci: true local_cache_directory: /cache local_cache_size_bytes: 50000000000 root_directory: /cache/remotebuilds/ api_key: "" ``` </details></details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary>🎫 <strong>No ticket provided </summary></strong> - [ ] Create ticket/issue  </details></td></tr> <tr><td colspan='2'><strong>Codebase Duplication Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>Codebase context is not defined </strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'><strong>Custom Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>No custom compliance provided</strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/'>guide</a> to enable custom compliance check. </details></td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant<br> 🟡 - Partial Compliant<br> 🔴 - Not Compliant<br> ⚪ - Requires Further Human Verification<br> 🏷️ - Compliance label<br> </details>

qodo-code-review[bot] commented

2025-10-07 02:56:26 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1728#issuecomment-3374981473
Original created: 2025-10-07T02:56:26Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category Suggestion Impact

Possible issue

Category	Suggestion	Impact
Possible issue	Use standard JSON marshaling for safety Replace the custom `escapeForJSON` function with the standard `json.Marshal` to ensure the generated preview string is always a valid JSON literal. pkg/core/flush.go [289-316] // truncateStatusDetailsIfTooLarge ensures a single service status record stays under the shard limit func (Server) truncateStatusDetailsIfTooLarge(status models.ServiceStatus, maxBytes int) { if status == nil \|\| len(status.Details) <= maxBytes { return } // Prepare a compact summary JSON with sizes and a short preview orig := status.Details previewLen := 256 if len(orig) < previewLen { previewLen = len(orig) } - summary := fmt.Sprintf(`{"truncated":true,"original_size":%d,"stored_size":%d,"preview":"%s"}`, + + // Use json.Marshal to safely create the preview string + previewBytes, err := json.Marshal(string(orig[:previewLen])) + if err != nil { + // Fallback to a simple message if marshaling fails + previewBytes = []byte(`"failed to marshal preview"`) + } + + summary := fmt.Sprintf(`{"truncated":true,"original_size":%d,"stored_size":%d,"preview":%s}`, len(orig), previewLen, - escapeForJSON(string(orig[:previewLen])), + string(previewBytes), ) status.Details = json.RawMessage(summary) } -// escapeForJSON escapes backslashes and quotes for embedding in JSON -func escapeForJSON(s string) string { - s = strings.ReplaceAll(s, `\\`, `\\\\`) - s = strings.ReplaceAll(s, `"`, `\\"`) - s = strings.ReplaceAll(s, "\n", `\n`) - s = strings.ReplaceAll(s, "\r", `\r`) - s = strings.ReplaceAll(s, "\t", `\t`) - return s -} - Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that manual JSON escaping is error-prone and proposes using the standard library's `json.Marshal` for a more robust and correct implementation.	Low
Update

Use standard JSON marshaling for safety

Replace the custom escapeForJSON function with the standard json.Marshal to
ensure the generated preview string is always a valid JSON literal.

pkg/core/flush.go [289-316]

 // truncateStatusDetailsIfTooLarge ensures a single service status record stays under the shard limit
 func (*Server) truncateStatusDetailsIfTooLarge(status *models.ServiceStatus, maxBytes int) {
     if status == nil || len(status.Details) <= maxBytes {
         return
     }
     // Prepare a compact summary JSON with sizes and a short preview
     orig := status.Details
     previewLen := 256
     if len(orig) < previewLen {
         previewLen = len(orig)
     }
-    summary := fmt.Sprintf(`{"truncated":true,"original_size":%d,"stored_size":%d,"preview":"%s"}`,
+
+    // Use json.Marshal to safely create the preview string
+    previewBytes, err := json.Marshal(string(orig[:previewLen]))
+    if err != nil {
+        // Fallback to a simple message if marshaling fails
+        previewBytes = []byte(`"failed to marshal preview"`)
+    }
+
+    summary := fmt.Sprintf(`{"truncated":true,"original_size":%d,"stored_size":%d,"preview":%s}`,
         len(orig),
         previewLen,
-        escapeForJSON(string(orig[:previewLen])),
+        string(previewBytes),
     )
     status.Details = json.RawMessage(summary)
 }
 
-// escapeForJSON escapes backslashes and quotes for embedding in JSON
-func escapeForJSON(s string) string {
-    s = strings.ReplaceAll(s, `\\`, `\\\\`)
-    s = strings.ReplaceAll(s, `"`, `\\"`)
-    s = strings.ReplaceAll(s, "\n", `\n`)
-    s = strings.ReplaceAll(s, "\r", `\r`)
-    s = strings.ReplaceAll(s, "\t", `\t`)
-    return s
-}
-

Apply / Chat

Suggestion importance[1-10]: 6

Why: The suggestion correctly identifies that manual JSON escaping is error-prone and proposes using the standard library's json.Marshal for a more robust and correct implementation.

Low

Update

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1728#issuecomment-3374981473 Original created: 2025-10-07T02:56:26Z --- ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion                                                                                                                                    </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Use standard JSON marshaling for safety</summary> ___ **Replace the custom <code>escapeForJSON</code> function with the standard <code>json.Marshal</code> to <br>ensure the generated preview string is always a valid JSON literal.** [pkg/core/flush.go [289-316]](https://github.com/carverauto/serviceradar/pull/1728/files#diff-4ad1cf4b0397bd277cd9c0553e4992ad7c9e9484d49d49d48af015958dc4bf4bR289-R316) ```diff // truncateStatusDetailsIfTooLarge ensures a single service status record stays under the shard limit func (*Server) truncateStatusDetailsIfTooLarge(status *models.ServiceStatus, maxBytes int) { if status == nil || len(status.Details) <= maxBytes { return } // Prepare a compact summary JSON with sizes and a short preview orig := status.Details previewLen := 256 if len(orig) < previewLen { previewLen = len(orig) } - summary := fmt.Sprintf(`{"truncated":true,"original_size":%d,"stored_size":%d,"preview":"%s"}`, + + // Use json.Marshal to safely create the preview string + previewBytes, err := json.Marshal(string(orig[:previewLen])) + if err != nil { + // Fallback to a simple message if marshaling fails + previewBytes = []byte(`"failed to marshal preview"`) + } + + summary := fmt.Sprintf(`{"truncated":true,"original_size":%d,"stored_size":%d,"preview":%s}`, len(orig), previewLen, - escapeForJSON(string(orig[:previewLen])), + string(previewBytes), ) status.Details = json.RawMessage(summary) } -// escapeForJSON escapes backslashes and quotes for embedding in JSON -func escapeForJSON(s string) string { - s = strings.ReplaceAll(s, `\\`, `\\\\`) - s = strings.ReplaceAll(s, `"`, `\\"`) - s = strings.ReplaceAll(s, "\n", `\n`) - s = strings.ReplaceAll(s, "\r", `\r`) - s = strings.ReplaceAll(s, "\t", `\t`) - return s -} - ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly identifies that manual JSON escaping is error-prone and proposes using the standard library's `json.Marshal` for a more robust and correct implementation. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td><td></td></tr></tbody></table>