Skip to content

Be less verbose in dnstap loop#154

Merged
eest merged 1 commit intomainfrom
less-logging-in-dnstap-loop
Jan 27, 2026
Merged

Be less verbose in dnstap loop#154
eest merged 1 commit intomainfrom
less-logging-in-dnstap-loop

Conversation

@eest
Copy link
Member

@eest eest commented Jan 26, 2026

Keep counters instead of potentially spamming logs.

Summary by CodeRabbit

  • Chores
    • Added three new per-instance monitoring counters to improve visibility into DNS processing issues (parsing errors, empty question sections, and invalid question names). Error handling now increments these metrics instead of logging, enabling better metrics-driven alerting and debugging.

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

@eest eest requested a review from a team as a code owner January 26, 2026 13:15
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 26, 2026

📝 Walkthrough

Walkthrough

The change replaces error logging with Prometheus counter increments in DNS dnstap parsing paths, adds three per-instance counters for parse errors, empty question sections, and invalid question names, and removes an unused loop index in question iteration.

Changes

Cohort / File(s) Summary
Prometheus instrumentation & parsing logic
pkg/runner/runner.go
Added three per-instance Prometheus counters (promDNSParseError, promEmptyQuestionSection, promInvalidQuestionName) and their initialization in newDnstapMinimiser. Replaced error logs with counter increments for parse failures, empty question sections, and invalid question names. Switched question loop from for i, question := range to for _, question := range (removed unused index).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 I bounced past logs and found a score,
Counters counting knocks upon the door,
Parse slips, blank queries, names gone stray,
Prometheus takes note as I hop away,
Metrics hum — I twitch my whiskers, hooray! 🎉

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Be less verbose in dnstap loop' accurately captures the main objective of reducing log verbosity in the dnstap processing loop by replacing error logs with Prometheus counters.

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

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

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

@eest eest requested a review from zluudg January 26, 2026 13:15
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@pkg/runner/runner.go`:
- Around line 1992-1996: The inner loop over msg.Question increments
edm.promInvalidQuestionName but uses a plain continue which only skips to the
next question and not the outer minimiserLoop, so messages with any invalid
question still get processed; change the control flow to skip the entire message
when an invalid question is found — either break out to a labeled continue for
minimiserLoop or set a local flag (e.g., invalidQuestion := true) when
edm.promInvalidQuestionName.Inc() is called and after the for _, question :=
range msg.Question loop check that flag and continue the outer loop
(minimiserLoop) to skip processing msg; update references to msg.Question and
edm.promInvalidQuestionName accordingly.

Keep counters instead of potentially spamming logs.

While here fix continue so it targets the correct loop so we skip
parsing in that case.
@eest eest force-pushed the less-logging-in-dnstap-loop branch from f72f9a6 to 8cf8854 Compare January 26, 2026 13:26
Copy link
Contributor

@zluudg zluudg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eest eest merged commit 266faa7 into main Jan 27, 2026
5 checks passed
@eest eest deleted the less-logging-in-dnstap-loop branch January 27, 2026 09:10
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.

2 participants