Added extended logging capability with metadata in addition to using … - #1827
Added extended logging capability with metadata in addition to using …#1827zbalkan wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds structured “extended logging” metadata for blocked DNS responses, allowing loggers/apps to record blocking context even when it isn’t present in the wire response.
Changes:
- Introduces
DnsQueryLogMetadata,DnsServerResponseMetadata, andIDnsQueryLoggerExto support metadata-aware logging. - Propagates response type + metadata via
DnsDatagram.Tag(including block-list-zone and app blocking responses) and updates stats/logging paths to read the new tag shape. - Extends bundled logging/exporter apps (SQL Server/SQLite/MySQL query logs, syslog exporter) to persist/emit blocking metadata.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| DnsServerCore/Dns/ZoneManagers/BlockListZoneManager.cs | Adds metadata generation for block-list-zone hits and attaches it to responses. |
| DnsServerCore/Dns/StatsManager.cs | Updates stats/log dispatch to derive response type and pass optional metadata to extended loggers. |
| DnsServerCore/Dns/DnsServer.cs | Propagates blocked-query metadata through blocked-query processing (incl. CNAME-cloaking path). |
| DnsServerCore.ApplicationCommon/IDnsQueryLogger.cs | Adds metadata model types + IDnsQueryLoggerEx and helpers for reading response tags. |
| Apps/QueryLogsSqlServerApp/App.cs | Adds blocking_metadata column and stores metadata JSON for blocked queries. |
| Apps/QueryLogsSqliteApp/App.cs | Adds blocking_metadata column and stores metadata JSON for blocked queries. |
| Apps/QueryLogsMySqlApp/App.cs | Adds blocking_metadata column and stores metadata JSON for blocked queries. |
| Apps/MispConnectorApp/App.cs | Attaches blocked-response metadata and includes blocking report text in EDE/TXT. |
| Apps/LogExporterApp/Strategy/SyslogExportStrategy.cs | Exports blocking metadata as structured syslog properties. |
| Apps/LogExporterApp/LogEntry.cs | Captures blocking metadata from logger arg/tag and improves EDE parsing robustness. |
| Apps/LogExporterApp/App.cs | Implements IDnsQueryLoggerEx and forwards metadata into queued log entries. |
| Apps/AdvancedBlockingApp/App.cs | Attaches metadata to blocked responses for advanced-blocking decisions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ac31104 to
aa9044d
Compare
…datagram contents
|
Updated to version 15.3 |
|
Responded to the review notes. No conflicts with the current version. |
|
Hi @ShreyasZare, Sory for insisting on the same page. Do you have time for a code review on this PR? |
|
Thanks for asking. I have not gone through the entire code but I still think that using the existing EDNS class for this would be much better than adding another metadata class. The blocking response being generated can be made to add EDNS for all requests and the same can be then used by logging apps. The EDNS info then can be removed just before responding to the actual request that does not support EDNS. |
|
Hi @ShreyasZare, While I agree that this is not a mandatory change or an obvious issue to deal with, I still see some value regarding the change. One thing worth flagging alongside the metadata design: both EDE and this PR's blocking metadata only cover deviation, by design, so allowed queries get no equivalent record. That means a silent log entry is ambiguous — was the query checked and passed, or did it never hit an evaluator at all? That ambiguity shows up worst during incident review (was this domain ever checked, against which blocklist snapshot?), and it also means a block-rate metric has no denominator — a blocklist that quietly stops matching looks identical in the logs to one that was never triggered. That said, I don't think we should just log everything by default. DNS logs are sensitive, and writing structured metadata on every successful query instead of just the blocked ones runs against minimization principles, and it's a real cost too — blocked queries are a small slice of total traffic, so multiplying row size and serialization work across SQL Server/SQLite/MySQL by the rest of the traffic isn't free. So my suggestion: keep this PR scoped to blocking, matching EDE's own scope, and treat "evaluated but not blocked" visibility as a separate opt-in advanced-logging feature later — a lightweight "evaluator ran, passed" marker rather than full metadata on every query. Keeps the default cheap and privacy-conservative, but gives operators who need the audit trail a way to get it. Happy to sketch that as a follow-up once we land the metadata-vs-EDNS question here. |
|
Thanks for the response. Its still early for me to say anything before analyzing the issue and the solution completely. I have this item marked in my list but currently have to work on a few things that are being prioritized before I can take up this specific task. |
|
Hi @ShreyasZare, No rush at all. Thanks for the update, and I appreciate you keeping it on your list. Happy to wait until you have time to look it over properly. |
…datagram contents.
Solving the extended logging issue discussed here: #1770