carverauto/serviceradar

Fork 0

linted #2327

Merged

mfreeman451 merged 1 commit from refs/pull/2327/head into main

2025-10-16 06:37:05 +00:00

mfreeman451 commented

2025-10-16 06:30:04 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #1783
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1783
Original created: 2025-10-16T06:30:04Z
Original updated: 2025-10-16T06:37:08Z
Original head: carverauto/serviceradar:1779-bugcore-agents-not-properly-registering-w-their-device-ip-address
Original base: main
Original merged: 2025-10-16T06:37:05Z by @mfreeman451

PR Type

Bug fix, Enhancement

Description

Enhanced agent device IP resolution with environment override and fallback logic
Enriched service status messages with host IP and hostname metadata
Made source_ip configuration optional with automatic detection fallback
Added comprehensive test coverage for new IP resolution and enrichment logic

Diagram Walkthrough

flowchart LR
  config["Configuration"] -- "source_ip optional" --> resolver["IP Resolver"]
  resolver -- "env override" --> agent["Agent Poller"]
  resolver -- "auto-detect" --> agent
  agent -- "device IP/host" --> check["Service Check"]
  check -- "enriched metadata" --> status["Service Status"]
  status -- "host_ip, hostname" --> core["Core Service"]

File Walkthrough

Relevant files

Enhancement

4 files

agent_poller.go `Added device IP resolution and message enrichment logic`	+234/-66
poller.go `Implemented source IP resolution and detection utilities`	+131/-0
results_poller.go `Applied message enrichment to results poller`	+1/-1
types.go `Added device IP and host fields to structs`	+24/-16

Tests

2 files

agent_poller_test.go `Added tests for IP resolution and message enrichment`	+82/-10
config_test.go `Added test for source_ip auto-default behavior`	+18/-1

Configuration changes

1 files

config.go
Made source_ip optional with auto fallback +1/-2

Formatting

3 files

serviceradar-config.yaml `Fixed indentation in agent configuration`	+2/-2
configmap.yaml `Fixed indentation in agent configuration`	+2/-2
poller-windows.json `Added newline at end of file`	+1/-1

Imported from GitHub pull request. Original GitHub pull request: #1783 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/1783 Original created: 2025-10-16T06:30:04Z Original updated: 2025-10-16T06:37:08Z Original head: carverauto/serviceradar:1779-bugcore-agents-not-properly-registering-w-their-device-ip-address Original base: main Original merged: 2025-10-16T06:37:05Z by @mfreeman451 --- ### **PR Type** Bug fix, Enhancement ___ ### **Description** - Enhanced agent device IP resolution with environment override and fallback logic - Enriched service status messages with host IP and hostname metadata - Made `source_ip` configuration optional with automatic detection fallback - Added comprehensive test coverage for new IP resolution and enrichment logic ___ ### Diagram Walkthrough ```mermaid flowchart LR config["Configuration"] -- "source_ip optional" --> resolver["IP Resolver"] resolver -- "env override" --> agent["Agent Poller"] resolver -- "auto-detect" --> agent agent -- "device IP/host" --> check["Service Check"] check -- "enriched metadata" --> status["Service Status"] status -- "host_ip, hostname" --> core["Core Service"] ``` <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>4 files</summary><table> <tr> <td><strong>agent_poller.go</strong><dd><code>Added device IP resolution and message enrichment logic</code>    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1783/files#diff-7cdaae416a1e7b5d39ad75f138c49fa0aca12fc38e8ab0b26bb59c64446da15c">+234/-66</a></td> </tr> <tr> <td><strong>poller.go</strong><dd><code>Implemented source IP resolution and detection utilities</code>  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1783/files#diff-28a10dea1596540e55ce9a8b68bd1af3d96bd4634f6def668643892cef25a086">+131/-0</a>  </td> </tr> <tr> <td><strong>results_poller.go</strong><dd><code>Applied message enrichment to results poller</code>                          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1783/files#diff-e9588a2564e1638097c9052d9a4cb8b43b992910770224b3a4210022f78c6b3e">+1/-1</a>      </td> </tr> <tr> <td><strong>types.go</strong><dd><code>Added device IP and host fields to structs</code>                              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1783/files#diff-0e185e2a77ce349c086c2d2d55033a3e271fcc1d7b16bb7c1d7c839f73e97e26">+24/-16</a>  </td> </tr> </table></details></td></tr><tr><td><strong>Tests</strong></td><td><details><summary>2 files</summary><table> <tr> <td><strong>agent_poller_test.go</strong><dd><code>Added tests for IP resolution and message enrichment</code>          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1783/files#diff-3b72f1c80958942904f082df21f74bb67bde710581174e9e0aa1dba82bd7971a">+82/-10</a>  </td> </tr> <tr> <td><strong>config_test.go</strong><dd><code>Added test for source_ip auto-default behavior</code>                      </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1783/files#diff-05ef22b193b3acfecc895fe1f364e852dcb2f3b08c606bc06c968cc88f0271e7">+18/-1</a>    </td> </tr> </table></details></td></tr><tr><td><strong>Configuration changes</strong></td><td><details><summary>1 files</summary><table> <tr> <td><strong>config.go</strong><dd><code>Made source_ip optional with auto fallback</code>                              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1783/files#diff-626ab53e2c4405a09e43c5ce9fb841a3cc83fec9dfa506366a4a8c2ecd490135">+1/-2</a>      </td> </tr> </table></details></td></tr><tr><td><strong>Formatting</strong></td><td><details><summary>3 files</summary><table> <tr> <td><strong>serviceradar-config.yaml</strong><dd><code>Fixed indentation in agent configuration</code>                                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1783/files#diff-b8c8d2484103b11c396bc60d290c81df63c30a0f81103eceb5852a17e1d2b5e3">+2/-2</a>      </td> </tr> <tr> <td><strong>configmap.yaml</strong><dd><code>Fixed indentation in agent configuration</code>                                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1783/files#diff-f4548beaa0a3a01a46971c82c5647a0f3f49eb38d66dd939d06d19018173fcd6">+2/-2</a>      </td> </tr> <tr> <td><strong>poller-windows.json</strong><dd><code>Added newline at end of file</code>                                                          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1783/files#diff-174a350e156bc34301652fb1803f91e51021dc887cd154bb2f0e520415233ebf">+1/-1</a>      </td> </tr> </table></details></td></tr></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-10-16 06:30:42 +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/1783#issuecomment-3409379415
Original created: 2025-10-16T06:30:42Z

You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

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

🟢

🎫 #1779

🟢	Ensure agents register/report with their correct device IP address.
	Use the poller-known IPs (from poller configuration/context) to populate agent IP when agents don’t convey it.
	Persist/enrich service status messages with host IP and hostname metadata so core receives it.
	Make source_ip configuration optional by providing an automatic detection or fallback.

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.

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/1783#issuecomment-3409379415 Original created: 2025-10-16T06:30:42Z --- _You are nearing your monthly Qodo Merge usage quota. For more information, please visit [here](https://qodo-merge-docs.qodo.ai/installation/qodo_merge/#cloud-users)._ ## 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/1779>#1779</a></summary> <table width='100%'><tbody> <tr><td rowspan=4>🟢</td> <td>Ensure agents register/report with their correct device IP address.</td></tr> <tr><td>Use the poller-known IPs (from poller configuration/context) to populate agent IP when <br>agents don’t convey it.</td></tr> <tr><td>Persist/enrich service status messages with host IP and hostname metadata so core receives <br>it.</td></tr> <tr><td>Make source_ip configuration optional by providing an automatic detection or fallback.</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>⚪</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">   </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-16 06:32:08 +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/1783#issuecomment-3409382600
Original created: 2025-10-16T06:32:08Z

You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Simplify the complex IP resolution logic The IP resolution logic is overly complex due to its use of multiple sources and fallbacks. It should be simplified to a more direct, prioritized approach for better maintainability. Examples: pkg/poller/agent_poller.go [96-183] `func resolveAgentHostMetadata(agentName string, config *AgentConfig, fallbackIP string, log logger.Logger) (ip string, host string) { candidates := make([]string, 0, 4) // Environment override takes precedence (allowing explicit configuration per agent) envKey := fmt.Sprintf("SERVICERADAR_AGENT_%s_IP", strings.ToUpper(strings.ReplaceAll(agentName, "-", "_"))) if envIP := os.Getenv(envKey); envIP != "" { candidates = append(candidates, envIP) } if config.Address != "" { ... (clipped 78 lines)` Solution Walkthrough: Before: func resolveAgentHostMetadata(agentName, config, fallbackIP): candidates = [] // 1. Add env var to candidates envKey = format("SERVICERADAR_AGENT_%s_IP", agentName) candidates.append(os.Getenv(envKey)) // 2. Add config address to candidates candidates.append(config.Address) // 3. Add agent name to candidates candidates.append(agentName) // 4. Add fallback IP to candidates candidates.append(fallbackIP) // Loop through all candidates and try to resolve an IP for candidate in candidates: if is_ip(candidate): ip = candidate break ips, err = dns_lookup(candidate) if err == nil and len(ips) > 0: ip = ips[0] break return ip, host After: func resolveAgentHostMetadata(agentName, config, fallbackIP): // 1. Environment variable override (highest priority) envKey = format("SERVICERADAR_AGENT_%s_IP", agentName) if envIP = os.Getenv(envKey); envIP != "": if parsedIP = net.ParseIP(envIP); parsedIP != nil: return parsedIP.String(), parsedIP.String() // 2. Explicit configuration (IP or resolvable host) if config.Address != "": host, _, _ = net.SplitHostPort(config.Address) if parsedIP = net.ParseIP(host); parsedIP != nil: return parsedIP.String(), host ips, err = dns_lookup(host) if err == nil and len(ips) > 0: return ips[0].String(), host // 3. Fallback to poller's IP if fallbackIP != "": return fallbackIP, agentName return "", agentName Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the new IP resolution logic in `resolveAgentHostMetadata` is complex and could be simplified to improve maintainability, which is a valid design concern for this PR's core logic.	Medium
Possible issue	Fix data loss when enriching status In `enrichPayloadWithHost`, modify the logic for handling non-map `status` fields. Instead of moving the original status to `status_text`, preserve it within the new `status` map under a `state` key to prevent data loss. pkg/poller/agent_poller.go [479-489] `var statusNode map[string]any switch current := payload["status"].(type) { case map[string]any: statusNode = current case nil: statusNode = make(map[string]any) default: - // Preserve original status text if present - payload["status_text"] = current + // Preserve original status text if present, and place it inside the new status node statusNode = make(map[string]any) + statusNode["state"] = current }` Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a logic flaw in `enrichPayloadWithHost` where the original status value is moved instead of being preserved within the `status` object, which could break downstream consumers. The proposed fix correctly preserves the data integrity.	Medium
Possible issue	Correct test to validate status preservation Update the assertions in `TestAgentPoller_ExecuteChecks_WithErrors` to reflect the correct behavior of `enrichPayloadWithHost`. The test should verify that the original status is preserved within the `status` object under a `state` key, not moved to `status_text`. pkg/poller/agent_poller_test.go [276-285] // Verify working service assert.True(t, workingStatus.Available) var workingPayload map[string]any require.NoError(t, json.Unmarshal(workingStatus.Message, &workingPayload)) assert.Equal(t, "127.0.0.1", workingPayload["host_ip"]) assert.Equal(t, "localhost", workingPayload["hostname"]) statusNode, ok := workingPayload["status"].(map[string]any) require.True(t, ok) assert.Equal(t, "127.0.0.1", statusNode["host_ip"]) - assert.Equal(t, "ok", workingPayload["status_text"]) + assert.Equal(t, "ok", statusNode["state"]) Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that the test `TestAgentPoller_ExecuteChecks_WithErrors` validates a flawed implementation. By proposing to update the test assertions to check for the correct behavior, it improves test quality and ensures the associated logic bug is fixed.	Medium
More

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1783#issuecomment-3409382600 Original created: 2025-10-16T06:32:08Z --- _You are nearing your monthly Qodo Merge usage quota. For more information, please visit [here](https://qodo-merge-docs.qodo.ai/installation/qodo_merge/#cloud-users)._ ## 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>High-level</td> <td> <details><summary>Simplify the complex IP resolution logic</summary> ___ **The IP resolution logic is overly complex due to its use of multiple sources and <br>fallbacks. It should be simplified to a more direct, prioritized approach for <br>better maintainability.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1783/files#diff-7cdaae416a1e7b5d39ad75f138c49fa0aca12fc38e8ab0b26bb59c64446da15cR96-R183">pkg/poller/agent_poller.go [96-183]</a> </summary> ```go func resolveAgentHostMetadata(agentName string, config *AgentConfig, fallbackIP string, log logger.Logger) (ip string, host string) { candidates := make([]string, 0, 4) // Environment override takes precedence (allowing explicit configuration per agent) envKey := fmt.Sprintf("SERVICERADAR_AGENT_%s_IP", strings.ToUpper(strings.ReplaceAll(agentName, "-", "_"))) if envIP := os.Getenv(envKey); envIP != "" { candidates = append(candidates, envIP) } if config.Address != "" { ... (clipped 78 lines) ``` </details> ### Solution Walkthrough: #### Before: ```go func resolveAgentHostMetadata(agentName, config, fallbackIP): candidates = [] // 1. Add env var to candidates envKey = format("SERVICERADAR_AGENT_%s_IP", agentName) candidates.append(os.Getenv(envKey)) // 2. Add config address to candidates candidates.append(config.Address) // 3. Add agent name to candidates candidates.append(agentName) // 4. Add fallback IP to candidates candidates.append(fallbackIP) // Loop through all candidates and try to resolve an IP for candidate in candidates: if is_ip(candidate): ip = candidate break ips, err = dns_lookup(candidate) if err == nil and len(ips) > 0: ip = ips[0] break return ip, host ``` #### After: ```go func resolveAgentHostMetadata(agentName, config, fallbackIP): // 1. Environment variable override (highest priority) envKey = format("SERVICERADAR_AGENT_%s_IP", agentName) if envIP = os.Getenv(envKey); envIP != "": if parsedIP = net.ParseIP(envIP); parsedIP != nil: return parsedIP.String(), parsedIP.String() // 2. Explicit configuration (IP or resolvable host) if config.Address != "": host, _, _ = net.SplitHostPort(config.Address) if parsedIP = net.ParseIP(host); parsedIP != nil: return parsedIP.String(), host ips, err = dns_lookup(host) if err == nil and len(ips) > 0: return ips[0].String(), host // 3. Fallback to poller's IP if fallbackIP != "": return fallbackIP, agentName return "", agentName ``` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies that the new IP resolution logic in `resolveAgentHostMetadata` is complex and could be simplified to improve maintainability, which is a valid design concern for this PR's core logic. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=2>Possible issue</td> <td> <details><summary>Fix data loss when enriching status</summary> ___ **In <code>enrichPayloadWithHost</code>, modify the logic for handling non-map <code>status</code> fields. <br>Instead of moving the original status to <code>status_text</code>, preserve it within the new <br><code>status</code> map under a <code>state</code> key to prevent data loss.** [pkg/poller/agent_poller.go [479-489]](https://github.com/carverauto/serviceradar/pull/1783/files#diff-7cdaae416a1e7b5d39ad75f138c49fa0aca12fc38e8ab0b26bb59c64446da15cR479-R489) ```diff var statusNode map[string]any switch current := payload["status"].(type) { case map[string]any: statusNode = current case nil: statusNode = make(map[string]any) default: - // Preserve original status text if present - payload["status_text"] = current + // Preserve original status text if present, and place it inside the new status node statusNode = make(map[string]any) + statusNode["state"] = current } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies a logic flaw in `enrichPayloadWithHost` where the original status value is moved instead of being preserved within the `status` object, which could break downstream consumers. The proposed fix correctly preserves the data integrity. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Correct test to validate status preservation</summary> ___ **Update the assertions in <code>TestAgentPoller_ExecuteChecks_WithErrors</code> to reflect the <br>correct behavior of <code>enrichPayloadWithHost</code>. The test should verify that the <br>original status is preserved within the <code>status</code> object under a <code>state</code> key, not <br>moved to <code>status_text</code>.** [pkg/poller/agent_poller_test.go [276-285]](https://github.com/carverauto/serviceradar/pull/1783/files#diff-3b72f1c80958942904f082df21f74bb67bde710581174e9e0aa1dba82bd7971aR276-R285) ```diff // Verify working service assert.True(t, workingStatus.Available) var workingPayload map[string]any require.NoError(t, json.Unmarshal(workingStatus.Message, &workingPayload)) assert.Equal(t, "127.0.0.1", workingPayload["host_ip"]) assert.Equal(t, "localhost", workingPayload["hostname"]) statusNode, ok := workingPayload["status"].(map[string]any) require.True(t, ok) assert.Equal(t, "127.0.0.1", statusNode["host_ip"]) - assert.Equal(t, "ok", workingPayload["status_text"]) + assert.Equal(t, "ok", statusNode["state"]) ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly points out that the test `TestAgentPoller_ExecuteChecks_WithErrors` validates a flawed implementation. By proposing to update the test assertions to check for the correct behavior, it improves test quality and ensures the associated logic bug is fixed. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> - [ ] More  </td><td></td></tr></tbody></table>