Skip to content

Added extended logging capability with metadata in addition to using … - #1827

Open
zbalkan wants to merge 3 commits into
TechnitiumSoftware:masterfrom
zbalkan:feat/extended-logging-with-metadata
Open

Added extended logging capability with metadata in addition to using …#1827
zbalkan wants to merge 3 commits into
TechnitiumSoftware:masterfrom
zbalkan:feat/extended-logging-with-metadata

Conversation

@zbalkan

@zbalkan zbalkan commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

…datagram contents.

Solving the extended logging issue discussed here: #1770

Copilot AI review requested due to automatic review settings April 9, 2026 11:10

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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, and IDnsQueryLoggerEx to 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.

Comment thread DnsServerCore.ApplicationCommon/IDnsQueryLogger.cs Outdated
Comment thread DnsServerCore/Dns/ZoneManagers/BlockListZoneManager.cs Outdated
Comment thread DnsServerCore/Dns/DnsServer.cs
Comment thread Apps/AdvancedBlockingApp/App.cs
@zbalkan zbalkan closed this May 11, 2026
@zbalkan
zbalkan force-pushed the feat/extended-logging-with-metadata branch from ac31104 to aa9044d Compare May 11, 2026 18:55
@zbalkan

zbalkan commented May 11, 2026

Copy link
Copy Markdown
Contributor Author

Updated to version 15.3

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

Comment thread DnsServerCore.ApplicationCommon/IDnsQueryLogger.cs
Comment thread DnsServerCore.ApplicationCommon/IDnsQueryLogger.cs
Comment thread DnsServerCore/Dns/DnsServer.cs Outdated
Comment thread DnsServerCore/Dns/ZoneManagers/BlockListZoneManager.cs
Comment thread DnsServerCore/Dns/ZoneManagers/BlockListZoneManager.cs Outdated
@zbalkan

zbalkan commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

Responded to the review notes. No conflicts with the current version.

@zbalkan

zbalkan commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Hi @ShreyasZare,

Sory for insisting on the same page. Do you have time for a code review on this PR?

@ShreyasZare

Copy link
Copy Markdown
Member

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.

@zbalkan

zbalkan commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

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.

@ShreyasZare

Copy link
Copy Markdown
Member

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.

@zbalkan

zbalkan commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

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.

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.

3 participants