Work to improve the minimizer - #262
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
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. Comment |
d2b6250 to
eec93a4
Compare
zluudg
left a comment
zluudg
left a comment
There was a problem hiding this comment.
So far I've identified three seemingly unrelated changes in this PR: migration of miekgs lib to a new version, change of golang version and formatting error strings. Maybe some things can be made into their own PRs to make reviewing easier. Like changing miekgs lib and the language version.
| "time" | ||
|
|
||
| "github.com/miekg/dns" | ||
| "codeberg.org/miekg/dns" |
There was a problem hiding this comment.
Why this switch? I'm not sure v2 of Miekg's lib is considered stable yet.
There was a problem hiding this comment.
Reason see elsewhere. In the README of the new library it states
- "Complete and usable DNS library."
- "We try to keep the "main" branch as sane as possible and at the bleeding edge of standards, avoiding breaking changes wherever reasonable. But because this version is young, we allow ourselves some more headroom for making backwards incompatible changes."
- "This new version will not soon see a v1.0.0 release because I want to be able to still make changes. In a year or two (2028?) when things have stabilized it will be blessed with a v1.0.0."
While the old version's README states "The version here will only see specific fixes and nothing else. At some point this repo will be archived."
|
|
||
| "github.com/miekg/dns" | ||
| "codeberg.org/miekg/dns" | ||
| dnsv1 "github.com/miekg/dns" |
There was a problem hiding this comment.
Using two versions of the same lib? Do we really want to go there?
There was a problem hiding this comment.
This is a temporary measure, some of the helper functions have changed slightly and I've not verified that they don't impose a problem for us. Therefore I've left then on the v1 version for the moment. I'm trying to do an incremental switch.
| go 1.26 | ||
|
|
||
| require ( | ||
| codeberg.org/miekg/dns v0.6.83 |
There was a problem hiding this comment.
I do not think we are ready for this migration of a core component. A strong motivation is needed.
There was a problem hiding this comment.
We have an issue of the EDM being slow, or in other words; using a lot of resources. Switching to the new version of miekg's dns library does help with that. Moreover the new version lets us do partial parsing of the dns message if we choose to. Today we don't use the Answers/Authorities/Additionals sections in the analysis.
Some benchmark of this example msg, the three first are comparable parsing the message fully. If we skip everything after the Questions section we can improve it even further(the last two lines). The third library is golang.org/x/net/dns/dnsmessage
BenchmarkMiekgV1-32 524742 2108 ns/op 672 B/op 22 allocs/op
BenchmarkMiekgV2-32 745074 1604 ns/op 520 B/op 16 allocs/op
BenchmarkGoDnsMessage-32 601540 1964 ns/op 2160 B/op 8 allocs/op
BenchmarkMiekgV2OnlyQuestions-32 2418212 509.7 ns/op 184 B/op 6 allocs/op
BenchmarkGoDnsMessageOnlyQuestions-32 2112134 560.2 ns/op 864 B/op 2 allocs/op
Sorry for the confusion! I'm using this draft PR to help myself keep track of things and trigger the GitHub actions. Not sure if there are better ways to do it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/runner/wkd.go (1)
174-186: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
Question.Qtypehere
msg.Question[0]is adns.Questionstruct, somsg.Question[0].(type)cannot match*dns.A/*dns.AAAA/*dns.MX/*dns.NS.MsgOptionUnpackQuestiononly parses the question section; it does not produce typed RR values there.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/runner/wkd.go` around lines 174 - 186, The question-type counting logic should use msg.Question[0].Qtype instead of a type switch on msg.Question[0]. Update the switch in the ClassINET branch to match the corresponding dns.TypeA, dns.TypeAAAA, dns.TypeMX, and dns.TypeNS constants, while retaining OtherTypeCount for all unmatched types.
🧹 Nitpick comments (2)
pkg/runner/session.go (2)
195-196: 🎯 Functional Correctness | 🔵 Trivial
// BUG?comment documents a potential pre-existing bug.The comment notes that
msg.Question[0].Nameis not normalized at this point innewSession. This aligns with the PR objective of documenting potential bugs. If the normalization is expected to happen elsewhere (per the "Normalizing query names only once" commit), consider clarifying in the comment where normalization occurs and whether this gap has downstream impact on label splitting. Would you like help investigating the normalization flow?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/runner/session.go` around lines 195 - 196, Clarify the BUG comment beside newSession’s edm.setLabels call by documenting where msg.Question[0].Name is normalized, or explicitly stating that no normalization occurs before dnsv1.SplitDomainName and noting the downstream label-splitting impact. Keep the comment focused on the normalization flow without changing behavior.
13-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
dns.SplitDomainNamehere Use thecodeberg.org/miekg/dnshelper instead ofdnsv1.SplitDomainName; it avoids keeping the extragithub.com/miekg/dnsimport around during the migration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/runner/session.go` around lines 13 - 15, Update the domain-name parsing call in the relevant session logic to use SplitDomainName from the already imported codeberg.org/miekg/dns package instead of dnsv1.SplitDomainName, then remove the now-unneeded github.com/miekg/dns alias import while preserving existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/runner/session_test.go`:
- Line 451: Update the test message setup to instantiate a dns.Msg with
new(dns.Msg), then initialize its question using SetQuestion for “example.com.”
and dns.TypeA. Replace the invalid dns.NewMsg call while preserving the existing
query values and subsequent test behavior.
- Line 451: Update the test setup around the dns message construction to replace
the nonexistent dns.NewMsg call with a dns.Msg instance initialized via
SetQuestion using the same domain and record type.
In `@pkg/runner/wkd_test.go`:
- Line 302: Keep the rcode test-case field used by the relevant test as int, or
explicitly convert it to int when assigning to dns.MsgHdr.Rcode; update the
assignment involving tc.rcode while preserving the existing dns.Rcode*
constants.
---
Outside diff comments:
In `@pkg/runner/wkd.go`:
- Around line 174-186: The question-type counting logic should use
msg.Question[0].Qtype instead of a type switch on msg.Question[0]. Update the
switch in the ClassINET branch to match the corresponding dns.TypeA,
dns.TypeAAAA, dns.TypeMX, and dns.TypeNS constants, while retaining
OtherTypeCount for all unmatched types.
---
Nitpick comments:
In `@pkg/runner/session.go`:
- Around line 195-196: Clarify the BUG comment beside newSession’s edm.setLabels
call by documenting where msg.Question[0].Name is normalized, or explicitly
stating that no normalization occurs before dnsv1.SplitDomainName and noting the
downstream label-splitting impact. Keep the comment focused on the normalization
flow without changing behavior.
- Around line 13-15: Update the domain-name parsing call in the relevant session
logic to use SplitDomainName from the already imported codeberg.org/miekg/dns
package instead of dnsv1.SplitDomainName, then remove the now-unneeded
github.com/miekg/dns alias import while preserving existing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 222a06af-1527-4cf3-b7bf-a6d6c3ac8330
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (30)
.github/workflows/container.ymlgo.modpkg/protocols/protocols.gopkg/protocols/protocols_test.gopkg/runner/atomic_reload_test.gopkg/runner/config_reload_test.gopkg/runner/data_collector.gopkg/runner/data_collector_test.gopkg/runner/disk_cleaner.gopkg/runner/disk_cleaner_test.gopkg/runner/dnstap_input.gopkg/runner/histogram.gopkg/runner/histogram_test.gopkg/runner/ignore.gopkg/runner/ignore_bench_test.gopkg/runner/ignore_test.gopkg/runner/manual_parquet_rotation_test.gopkg/runner/minimiser.gopkg/runner/minimiser_test.gopkg/runner/mqtt.gopkg/runner/mqtt_test.gopkg/runner/qname_seen.gopkg/runner/qname_seen_bench_test.gopkg/runner/qname_seen_test.gopkg/runner/service.gopkg/runner/session.gopkg/runner/session_test.gopkg/runner/test_helpers_test.gopkg/runner/wkd.gopkg/runner/wkd_test.go
d3871b5 to
16c0b3e
Compare
Expexted impact: none
improve performance by only calling strings.ToLower once per qname Expected impact: none
…s package
Expected impact: since they are different versions, they may come with
different idiosyncrasies that may impact statistics
only unpack until and including the question section of the
dns message, ie. not the answers
Expected impact: changing statistics, since this will allow through DNS
messages which are not fully parsable compared to
before when these were rejected
16c0b3e to
ed06979
Compare
2 participants
NOT FOR MERGING!
The goal is to improve the maintainability and performance of the minimiser one step at a time.
Upgrading to newer version of go to enable some new go apis.
Some parts may have impact on the statistics, see commit messages
Also one commit adds comments of potential bugs from before...
Builds on #243
Summary by CodeRabbit