Skip to content

feat(packetloss): add option to enable DNS resolution in MTR tests#172

Merged
s0up4200 merged 1 commit into
developfrom
feat/issue-160-mtr-dns-resolution
Jan 24, 2026
Merged

feat(packetloss): add option to enable DNS resolution in MTR tests#172
s0up4200 merged 1 commit into
developfrom
feat/issue-160-mtr-dns-resolution

Conversation

@s0up4200

@s0up4200 s0up4200 commented Jan 24, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Adds mtr_enable_dns config option to the [packetloss] section that controls whether MTR tests resolve hostnames or show raw IP addresses
  • Disabled by default to preserve current behavior (fast, numeric-only output)
  • When enabled, removes --no-dns (Unix) / -n (Windows) flags from MTR commands so hostnames like one.one.one.one are shown instead of 1.1.1.1
  • Configurable via config file (mtr_enable_dns = true) or environment variable (NETRONOME__PACKETLOSS_MTR_ENABLE_DNS=true)

Closes #160

Test plan

  • go build ./... compiles without errors
  • go test ./... all tests pass
  • Start with mtr_enable_dns = false (default) and verify MTR results show IP addresses only
  • Start with mtr_enable_dns = true and verify MTR results show resolved hostnames
  • Set NETRONOME__PACKETLOSS_MTR_ENABLE_DNS=true env var and verify it overrides the config file
  • Run generate-config and verify the new option appears in the generated config

Summary by CodeRabbit

  • New Features
    • Added configuration option to control DNS lookup behavior during packet loss monitoring tests, allowing users to toggle DNS resolution checks during network diagnostics.

✏️ Tip: You can customize this high-level summary in your review settings.

Add mtr_enable_dns config option to show hostnames instead of IP
addresses in MTR traceroute results. Disabled by default to preserve
current behavior.

Configurable via:
- config.toml: [packetloss] mtr_enable_dns = true
- Environment variable: NETRONOME__PACKETLOSS_MTR_ENABLE_DNS=true

Closes #160
@coderabbitai

coderabbitai Bot commented Jan 24, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

This PR adds a new mtr_enable_dns configuration option to allow users to enable DNS resolution in MTR tests. The flag is integrated through the configuration system, passed to the PacketLossService, and conditionally applied to MTR argument building on both Unix and Windows platforms.

Changes

Cohort / File(s) Summary
Configuration Schema
config.toml, internal/config/config.go
Introduces new mtr_enable_dns boolean field (default: false) to PacketLossConfig with TOML tag mtr_enable_dns and environment variable PACKETLOSS_MTR_ENABLE_DNS. Updated configuration initialization and serialization to support the new option.
Service Initialization
cmd/netronome/main.go, internal/speedtest/packetloss.go
PacketLossService constructor now accepts enableDNS boolean parameter. Service constructor call in main.go passes cfg.PacketLoss.MTREnableDNS when initializing the service. New field added to PacketLossService struct to store the flag.
MTR Argument Building
internal/speedtest/mtr_unix.go, internal/speedtest/mtr_windows.go
Updated buildMTRArgs function signatures on both platforms to accept enableDNS parameter. Changed conditional logic: --no-dns (Unix) and -n (Windows) flags are now only appended when DNS is disabled, rather than being unconditionally included.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A config flag hops through the code,
DNS names now show the network road,
Hostnames glow where IPs once stood,
MTR sees the path—oh, so good! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: adding a DNS resolution option to MTR tests for packet loss monitoring.
Linked Issues check ✅ Passed The implementation fully addresses issue #160 by adding mtr_enable_dns config option with both file and environment variable support, and properly controls DNS behavior in MTR.
Out of Scope Changes check ✅ Passed All changes are directly related to implementing the MTR DNS resolution feature requested in issue #160; no unrelated modifications detected.
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Comment @coderabbitai help to get the list of available commands and usage tips.

@s0up4200 s0up4200 merged commit 07b4601 into develop Jan 24, 2026
11 checks passed
@s0up4200 s0up4200 deleted the feat/issue-160-mtr-dns-resolution branch January 24, 2026 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add option to enable DNS resolution in MTR tests

1 participant