carverauto/serviceradar

Fork 0

2069 bug icmp metrics missing in k8s #2514

Merged

mfreeman451 merged 15 commits from refs/pull/2514/head into main

2025-12-08 02:41:39 +00:00

mfreeman451 commented

2025-12-07 01:00:40 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2070
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2070
Original created: 2025-12-07T01:00:40Z
Original updated: 2025-12-08T02:41:43Z
Original head: carverauto/serviceradar:2069-bug-icmp-metrics-missing-in-k8s
Original base: main
Original merged: 2025-12-08T02:41:39Z by @mfreeman451

User description

IMPORTANT: Please sign the Developer Certificate of Origin

Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include
a DCO sign-off statement indicating the DCO acceptance in one commit message. Here
is an example DCO Signed-off-by line in a commit message:

Signed-off-by: J. Doe <j.doe@domain.com>

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

I have signed the DCO?
The build completes without errors?
All tests are passing when running make test?

PR Type

Bug fix

Description

Fix ICMP metrics attribution to agent service devices in Kubernetes
Preserve target device/host identifiers in metadata instead of overriding primary device ID
Normalize agent/placeholder identifiers to prevent capability snapshot misattribution
Add regression test for ICMP payloads with device_id and agent_id

Diagram Walkthrough

flowchart LR
  A["ICMP Payload<br/>with device_id"] -->|Previously| B["Override to<br/>payload device_id"]
  A -->|Now| C["Keep agent<br/>service device"]
  C -->|Store in| D["Metadata:<br/>target_device_id"]
  B -->|Result| E["Missing ICMP<br/>metrics in UI"]
  C -->|Result| F["ICMP sparklines<br/>visible"]

File Walkthrough

Relevant files

Bug fix

metrics.go

Preserve target device ID in metadata

pkg/core/metrics.go

Separate target device ID from resolved device ID to preserve payload
hints in metadata
Add target_device_id to metric, device, and event metadata when
present
Refactor device resolution logic to prioritize agent ID over payload
device ID
Trim whitespace from agentID and partition parameters in
resolveICMPDevice

+18/-7

Tests

metrics_test.go

Add ICMP payload device ID regression test

pkg/core/metrics_test.go

Add regression test
TestProcessICMPMetricsIgnoresPayloadDeviceIDWhenAgentPresent
Verify ICMP metrics attach to agent service device when both device_id
and agent_id present
Confirm target device ID is preserved in metric metadata
Validate device update attributes match agent identity

+55/-0

Documentation

CLAUDE.md `Add OpenSpec AI assistant instructions` CLAUDE.md Add OpenSpec instructions for AI assistants Reference `@/openspec/AGENTS.md` for change proposals and specifications Document when to consult authoritative specs before coding	+18/-0
proposal.md `Add change proposal for ICMP attribution fix` openspec/changes/fix-icmp-service-device-attribution/proposal.md Document root cause of missing ICMP sparklines in Kubernetes agents Explain how placeholder `device_id` values cause capability snapshot misattribution Define solution to preserve target identifiers in metadata Specify impact on service-device-capabilities specification	+13/-0
spec.md `Add service device capabilities specification` openspec/changes/fix-icmp-service-device-attribution/specs/service-device-capabilities/spec.md Add requirement for agent ICMP results to attach to service devices Define scenario where payload `device_id` does not override agent attribution Add requirement for placeholder host identifiers to not block ICMP visibility Specify fallback behavior for non-IP host identifiers	+14/-0
tasks.md `Add implementation tasks for ICMP fix` openspec/changes/fix-icmp-service-device-attribution/tasks.md Define implementation tasks for agent ICMP response attribution Specify normalization of placeholder host identifiers Require regression test coverage for device_id and agent_id combinations Verify device and metrics endpoints expose ICMP capability for agents	+5/-0

Configuration changes

values.yaml

Update application image tag

helm/serviceradar/values.yaml

Update Docker image tag to reflect bug fix deployment

+1/-1

Imported from GitHub pull request. Original GitHub pull request: #2070 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2070 Original created: 2025-12-07T01:00:40Z Original updated: 2025-12-08T02:41:43Z Original head: carverauto/serviceradar:2069-bug-icmp-metrics-missing-in-k8s Original base: main Original merged: 2025-12-08T02:41:39Z by @mfreeman451 --- ### **User description** ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test? ___ ### **PR Type** Bug fix ___ ### **Description** - Fix ICMP metrics attribution to agent service devices in Kubernetes - Preserve target device/host identifiers in metadata instead of overriding primary device ID - Normalize agent/placeholder identifiers to prevent capability snapshot misattribution - Add regression test for ICMP payloads with device_id and agent_id ___ ### Diagram Walkthrough ```mermaid flowchart LR A["ICMP Payload<br/>with device_id"] -->|Previously| B["Override to<br/>payload device_id"] A -->|Now| C["Keep agent<br/>service device"] C -->|Store in| D["Metadata:<br/>target_device_id"] B -->|Result| E["Missing ICMP<br/>metrics in UI"] C -->|Result| F["ICMP sparklines<br/>visible"] ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix</strong></td><td><table> <tr> <td> <details> <summary><strong>metrics.go</strong><dd><code>Preserve target device ID in metadata</code>                                        </dd></summary> <hr> pkg/core/metrics.go <ul><li>Separate target device ID from resolved device ID to preserve payload <br>hints in metadata<br> <li> Add <code>target_device_id</code> to metric, device, and event metadata when <br>present<br> <li> Refactor device resolution logic to prioritize agent ID over payload <br>device ID<br> <li> Trim whitespace from <code>agentID</code> and <code>partition</code> parameters in <br><code>resolveICMPDevice</code></ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2070/files#diff-6d98e853ce17576c088e77956ae4ecfa8078019e0bff107a79d8d1d6ed2443ad">+18/-7</a>    </td> </tr> </table></td></tr><tr><td><strong>Tests</strong></td><td><table> <tr> <td> <details> <summary><strong>metrics_test.go</strong><dd><code>Add ICMP payload device ID regression test</code>                              </dd></summary> <hr> pkg/core/metrics_test.go <ul><li>Add regression test <br><code>TestProcessICMPMetricsIgnoresPayloadDeviceIDWhenAgentPresent</code><br> <li> Verify ICMP metrics attach to agent service device when both <code>device_id</code> <br>and <code>agent_id</code> present<br> <li> Confirm target device ID is preserved in metric metadata<br> <li> Validate device update attributes match agent identity</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2070/files#diff-2b0ce06b068be7b4418c3fe4d23e3b8bf536d0cc2b9201a6949976e298a9e95e">+55/-0</a>    </td> </tr> </table></td></tr><tr><td><strong>Documentation</strong></td><td><table> <tr> <td> <details> <summary><strong>CLAUDE.md</strong><dd><code>Add OpenSpec AI assistant instructions</code>                                      </dd></summary> <hr> CLAUDE.md <ul><li>Add OpenSpec instructions for AI assistants<br> <li> Reference <code>@/openspec/AGENTS.md</code> for change proposals and specifications<br> <li> Document when to consult authoritative specs before coding</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2070/files#diff-6ebdb617a8104a7756d0cf36578ab01103dc9f07e4dc6feb751296b9c402faf7">+18/-0</a>    </td> </tr> <tr> <td> <details> <summary><strong>proposal.md</strong><dd><code>Add change proposal for ICMP attribution fix</code>                          </dd></summary> <hr> openspec/changes/fix-icmp-service-device-attribution/proposal.md <ul><li>Document root cause of missing ICMP sparklines in Kubernetes agents<br> <li> Explain how placeholder <code>device_id</code> values cause capability snapshot <br>misattribution<br> <li> Define solution to preserve target identifiers in metadata<br> <li> Specify impact on service-device-capabilities specification</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2070/files#diff-ca1b0821c2554f35858d70b8a41cd51bac6611ac04e7e16dfad75f38ed2ce930">+13/-0</a>    </td> </tr> <tr> <td> <details> <summary><strong>spec.md</strong><dd><code>Add service device capabilities specification</code>                        </dd></summary> <hr> openspec/changes/fix-icmp-service-device-attribution/specs/service-device-capabilities/spec.md <ul><li>Add requirement for agent ICMP results to attach to service devices<br> <li> Define scenario where payload <code>device_id</code> does not override agent <br>attribution<br> <li> Add requirement for placeholder host identifiers to not block ICMP <br>visibility<br> <li> Specify fallback behavior for non-IP host identifiers</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2070/files#diff-69f23ca388bf44599fe256a2dedafc0e8af9fb346bb917efe6f2c95b84e971d7">+14/-0</a>    </td> </tr> <tr> <td> <details> <summary><strong>tasks.md</strong><dd><code>Add implementation tasks for ICMP fix</code>                                        </dd></summary> <hr> openspec/changes/fix-icmp-service-device-attribution/tasks.md <ul><li>Define implementation tasks for agent ICMP response attribution<br> <li> Specify normalization of placeholder host identifiers<br> <li> Require regression test coverage for device_id and agent_id <br>combinations<br> <li> Verify device and metrics endpoints expose ICMP capability for agents</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2070/files#diff-0d3a5a254abeadabd0c30d04b56b43c8f4b43dcdb6c8f300d1f953fa602247d7">+5/-0</a>      </td> </tr> </table></td></tr><tr><td><strong>Configuration changes</strong></td><td><table> <tr> <td> <details> <summary><strong>values.yaml</strong><dd><code>Update application image tag</code>                                                          </dd></summary> <hr> helm/serviceradar/values.yaml - Update Docker image tag to reflect bug fix deployment </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2070/files#diff-d4449c7cb70362554b274f81eae5a4b81a8e81df494282e383d1b7ea3871c452">+1/-1</a>      </td> </tr> </table></td></tr></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-12-07 01:01:15 +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/2070#issuecomment-3621442413
Original created: 2025-12-07T01:01:15Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance

🟢

No security concerns identified

No security vulnerabilities detected by AI analysis. Human verification advised for critical code.

Ticket Compliance

🟡

🎫 #2069

🟢	Fix attribution so ICMP results collected by an agent are associated with the agent service device, not a placeholder or target device.
	Preserve target host/device identifiers as metadata for observability rather than overriding the primary device ID.
	Provide regression tests to prevent reintroduction of the missing ICMP metrics issue when payload contains both device_id and agent_id.
	Ensure behavior works in Kubernetes deployment (Helm values reflect deployable version with the fix).
⚪	ICMP metrics must be present for k8s agents (e.g., k8s-agent) in the demo namespace, appearing in device inventory sparklines and device details.

Codebase Duplication Compliance

⚪

Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance

🟢

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

⚪

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Action Logging: New ICMP processing paths add metadata and device resolution but do not explicitly log
critical attribution decisions (e.g., preferring agent device ID over payload device_id),
which may hinder auditability of critical system actions.

Referred Code

) (string, string, string, string, error) {
	agentID = strings.TrimSpace(agentID)
	partition = strings.TrimSpace(partition)
	collectorIP := normalizeHostIP(s.resolveServiceHostIP(ctx, pollerID, agentID, sourceIP))
	if collectorIP == "" {
		collectorIP = normalizeHostIP(sourceIP)
	}
	if collectorIP != "" && net.ParseIP(collectorIP) == nil {
		collectorIP = ""
	}

	resolvedDeviceID := strings.TrimSpace(deviceID)
	if agentID != "" {
		resolvedDeviceID = models.GenerateServiceDeviceID(models.ServiceTypeAgent, agentID)
	} else if resolvedDeviceID == "" {
		if collectorIP != "" {
			resolvedDeviceID = models.GenerateNetworkDeviceID(partition, collectorIP)
		} else {
			return "", "", collectorIP, "", errICMPDeviceIdentifiersMissing
		}
	}

Learn more about managing compliance generic rules or creating your own custom rules

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/2070#issuecomment-3621442413 Original created: 2025-12-07T01:01:15Z --- ## 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>🟢</td><td><details><summary><strong>No security concerns identified</strong></summary> No security vulnerabilities detected by AI analysis. Human verification advised for critical code. </details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>🟡</td> <td> <details> <summary>🎫 <a href=https://github.com/carverauto/serviceradar/issues/2069>#2069</a></summary> <table width='100%'><tbody> <tr><td rowspan=4>🟢</td> <td>Fix attribution so ICMP results collected by an agent are associated with the agent <br>service device, not a placeholder or target device.</td></tr> <tr><td>Preserve target host/device identifiers as metadata for observability rather than <br>overriding the primary device ID.</td></tr> <tr><td>Provide regression tests to prevent reintroduction of the missing ICMP metrics issue when <br>payload contains both device_id and agent_id.</td></tr> <tr><td>Ensure behavior works in Kubernetes deployment (Helm values reflect deployable version <br>with the fix).</td></tr> <tr><td rowspan=1>⚪</td> <td>ICMP metrics must be present for k8s agents (e.g., k8s-agent) in the demo namespace, <br>appearing in device inventory sparklines and device details.</td></tr> </tbody></table> </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 rowspan=5>🟢</td><td> <details><summary><strong>Generic: Meaningful Naming and Self-Documenting Code</strong></summary><br> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code <br>self-documenting<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Robust Error Handling and Edge Case Management</strong></summary><br> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful <br>degradation<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Secure Error Handling</strong></summary><br> **Objective:** To prevent the leakage of sensitive system information through error messages while <br>providing sufficient detail for internal debugging.<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Secure Logging Practices</strong></summary><br> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive <br>information like PII, PHI, or cardholder data.<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Security-First Input Validation and Data Handling</strong></summary><br> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent <br>vulnerabilities<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=1>⚪</td> <td><details> <summary><strong>Generic: Comprehensive Audit Trails</strong></summary><br> **Objective:** To create a detailed and reliable record of critical system actions for security analysis <br>and compliance.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2070/files#diff-6d98e853ce17576c088e77956ae4ecfa8078019e0bff107a79d8d1d6ed2443adR820-R840'><strong>Action Logging</strong></a>: New ICMP processing paths add metadata and device resolution but do not explicitly log <br>critical attribution decisions (e.g., preferring agent device ID over payload device_id), <br>which may hinder auditability of critical system actions.<br> <details open><summary>Referred Code</summary> ```go ) (string, string, string, string, error) { agentID = strings.TrimSpace(agentID) partition = strings.TrimSpace(partition) collectorIP := normalizeHostIP(s.resolveServiceHostIP(ctx, pollerID, agentID, sourceIP)) if collectorIP == "" { collectorIP = normalizeHostIP(sourceIP) } if collectorIP != "" && net.ParseIP(collectorIP) == nil { collectorIP = "" } resolvedDeviceID := strings.TrimSpace(deviceID) if agentID != "" { resolvedDeviceID = models.GenerateServiceDeviceID(models.ServiceTypeAgent, agentID) } else if resolvedDeviceID == "" { if collectorIP != "" { resolvedDeviceID = models.GenerateNetworkDeviceID(partition, collectorIP) } else { return "", "", collectorIP, "", errICMPDeviceIdentifiersMissing } } ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </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-12-07 01:02:04 +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/2070#issuecomment-3621443130
Original created: 2025-12-07T01:02:04Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Improve device resolution for non-IP placeholders In `resolveICMPDevice`, when `agentID` is not present, add a check to ensure the payload `deviceID` is a valid IP address before using it, falling back to `collectorIP` if it is not. pkg/core/metrics.go [831-840] `resolvedDeviceID := strings.TrimSpace(deviceID) if agentID != "" { resolvedDeviceID = models.GenerateServiceDeviceID(models.ServiceTypeAgent, agentID) - } else if resolvedDeviceID == "" { + } else if resolvedDeviceID == "" \|\| net.ParseIP(resolvedDeviceID) == nil { if collectorIP != "" { resolvedDeviceID = models.GenerateNetworkDeviceID(partition, collectorIP) } else { return "", "", collectorIP, "", errICMPDeviceIdentifiersMissing } }` Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a gap in handling non-IP placeholder `deviceID` values when `agentID` is absent, which aligns with the PR's goal to fix device attribution issues.	Medium
General	Use a valid IP for test accuracy In `TestProcessICMPMetricsIgnoresPayloadDeviceIDWhenAgentPresent`, replace the non-IP `source_ip` argument `"agent"` with a valid IP address like `"10.1.2.3"` to better simulate a real-world scenario. pkg/core/metrics_test.go [516-517] `- err := server.processICMPMetrics(ctx, "k8s-poller", "default", "agent", "k8s-agent", svc, payload, now) + err := server.processICMPMetrics(ctx, "k8s-poller", "default", "10.1.2.3", "k8s-agent", svc, payload, now) require.NoError(t, err)` Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that using a non-IP string for `source_ip` is unrealistic and improves the test's robustness by using a valid IP address.	Medium
Update

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2070#issuecomment-3621443130 Original created: 2025-12-07T01:02:04Z --- ## 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>Improve device resolution for non-IP placeholders</summary> ___ **In <code>resolveICMPDevice</code>, when <code>agentID</code> is not present, add a check to ensure the <br>payload <code>deviceID</code> is a valid IP address before using it, falling back to <br><code>collectorIP</code> if it is not.** [pkg/core/metrics.go [831-840]](https://github.com/carverauto/serviceradar/pull/2070/files#diff-6d98e853ce17576c088e77956ae4ecfa8078019e0bff107a79d8d1d6ed2443adR831-R840) ```diff resolvedDeviceID := strings.TrimSpace(deviceID) if agentID != "" { resolvedDeviceID = models.GenerateServiceDeviceID(models.ServiceTypeAgent, agentID) - } else if resolvedDeviceID == "" { + } else if resolvedDeviceID == "" || net.ParseIP(resolvedDeviceID) == nil { if collectorIP != "" { resolvedDeviceID = models.GenerateNetworkDeviceID(partition, collectorIP) } else { return "", "", collectorIP, "", errICMPDeviceIdentifiersMissing } } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly identifies a gap in handling non-IP placeholder `deviceID` values when `agentID` is absent, which aligns with the PR's goal to fix device attribution issues. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=1>General</td> <td> <details><summary>Use a valid IP for test accuracy</summary> ___ **In <code>TestProcessICMPMetricsIgnoresPayloadDeviceIDWhenAgentPresent</code>, replace the <br>non-IP <code>source_ip</code> argument <code>"agent"</code> with a valid IP address like <code>"10.1.2.3"</code> to <br>better simulate a real-world scenario.** [pkg/core/metrics_test.go [516-517]](https://github.com/carverauto/serviceradar/pull/2070/files#diff-2b0ce06b068be7b4418c3fe4d23e3b8bf536d0cc2b9201a6949976e298a9e95eR516-R517) ```diff - err := server.processICMPMetrics(ctx, "k8s-poller", "default", "agent", "k8s-agent", svc, payload, now) + err := server.processICMPMetrics(ctx, "k8s-poller", "default", "10.1.2.3", "k8s-agent", svc, payload, now) require.NoError(t, err) ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly points out that using a non-IP string for `source_ip` is unrealistic and improves the test's robustness by using a valid IP address. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td><td></td></tr></tbody></table>