Feat dns interceptor storage #4589

SuperOleg39 · 2025-09-26T09:57:36Z

Rationale

Alternative implementation for #4565

Custom cache for DNS interceptor allows:

add custom metrics for DNS cache hit rate monitoring
use any cache library (lru-cache, etc.)

Reference - https://github.com/szmarczak/cacheable-lookup?tab=readme-ov-file#cache

Changes

Added cache parameter for DNSInstance

Status

SuperOleg39 · 2025-09-26T10:01:07Z

lib/interceptor/dns.js

+    this.#records.delete(hostname)
+  }
+
+  // Delegate to storage decide can we do more lookups or not


For example, lru-cache adapter can always return false here

SuperOleg39 · 2025-09-26T10:01:54Z

types/interceptors.d.ts

  // DNS interceptor
  export type DNSInterceptorRecord = { address: string, ttl: number, family: 4 | 6 }
-  export type DNSInterceptorOriginRecords = { 4: { ips: DNSInterceptorRecord[] } | null, 6: { ips: DNSInterceptorRecord[] } | null }
+  export type DNSInterceptorOriginRecords = { records: { 4: { ips: DNSInterceptorRecord[] } | null, 6: { ips: DNSInterceptorRecord[] } | null } }


Here was incorrect typings - cached records shape is { records: ... }

SuperOleg39 · 2025-09-26T10:02:24Z

types/interceptors.d.ts

    maxTTL?: number
    maxItems?: number
-    lookup?: (hostname: string, options: LookupOptions, callback: (err: NodeJS.ErrnoException | null, addresses: DNSInterceptorRecord[]) => void) => void
+    lookup?: (origin: URL, options: LookupOptions, callback: (err: NodeJS.ErrnoException | null, addresses: DNSInterceptorRecord[]) => void) => void


Here also was incorrect typings - before pick method calls, string origin is converted to URL

SuperOleg39 · 2025-09-26T10:08:50Z

lib/interceptor/dns.js

 const { InvalidArgumentError, InformationalError } = require('../core/errors')
 const maxInt = Math.pow(2, 31) - 1

+class DNSStorage {


lru-cache adapter example:

const cache = new LRU({ max: maxItems }) const adapter = { set: (hostname: string, records: any, opts: { ttl: number }): void => { cache.set(hostname, records, opts); }, get: (hostname: string): any => { return cache.get(hostname); }, delete: (hostname: string) => { cache.delete(hostname); }, full: (): boolean => { return false; }, }; const interceptor = interceptors.dns({ maxItems: Infinity, maxTTL: 60000, cache: adapter })

Also, if you will be interesting about lru-cache with integrated prometheus metrics, here is our implementation:

adapter - https://github.com/tramvaijs/tramvai/blob/main/packages/modules/common/src/cache/cacheWithMetricsProxy.ts

metrics - https://github.com/tramvaijs/tramvai/blob/main/packages/modules/common/src/cache/cacheMetrics.ts#L22

Grafana visualization:

Cache metrics was a veeeery useful feature for us. But mostly for HTTP requests ofc)

SuperOleg39 · 2025-09-26T14:22:04Z

lib/interceptor/dns.js

      if (typeof record.ttl === 'number') {
        // The record TTL is expected to be in ms
        record.ttl = Math.min(record.ttl, this.#maxTTL)
+        minTTL = Math.min(minTTL, record.ttl)


For LRU or any other caches, which will not be able to respect ttl per record, just pass the minimum value from records.

Can't find better solution because records is a mutable object and we need to store the same for every origin,

It should be fine to keep it as is, this just duplicates it.

The idea is to find minimum TTL for all resolved records for origin, and pass it to external storage, I don't quite understand about duplication

ah ok see it now, missed that bit

Copilot

Pull Request Overview

This PR adds custom storage support for the DNS interceptor, allowing users to provide their own cache implementation instead of relying on the built-in Map-based storage. This enables integration with external cache libraries like lru-cache and provides better control over caching behavior including custom metrics collection.

Introduces a configurable storage parameter for DNS interceptor that accepts custom cache implementations
Refactors internal DNS storage logic into a separate DNSStorage class while maintaining backward compatibility
Adds comprehensive validation for custom storage interface requirements

Reviewed Changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.

File	Description
lib/interceptor/dns.js	Implements the core storage abstraction with DNSStorage class and storage parameter validation
test/interceptors/dns.js	Adds comprehensive test coverage for external storage functionality
docs/docs/api/Dispatcher.md	Documents the new storage parameter with usage examples including LRU cache integration

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-26T19:42:43Z

lib/interceptor/dns.js


-    this.#records.set(origin.hostname, records)
+    // We provide a default TTL if external storage will be used without TTL per record-level support
+    this.storage.set(origin.hostname, records, { ttl: minTTL })


The storage interface documentation shows that the ttl parameter is optional, but this code always passes it. Consider checking if the storage implementation supports TTL before passing the options parameter to avoid potential issues with storage implementations that don't expect additional parameters.

Suggested change

this.storage.set(origin.hostname, records, { ttl: minTTL })

if (typeof this.storage.set === 'function' && this.storage.set.length >= 3) {

this.storage.set(origin.hostname, records, { ttl: minTTL })

} else {

this.storage.set(origin.hostname, records)

}

The third argument is not used isn't it?

It can be used in external storages, if lru-cache or quick-lru will be used under the hood

Copilot · 2025-09-26T19:42:43Z

lib/interceptor/dns.js

  setRecords (origin, addresses) {
    const timestamp = Date.now()
    const records = { records: { 4: null, 6: null } }
+    let minTTL = this.#maxTTL


The minTTL initialization should use Infinity instead of this.#maxTTL to correctly calculate the minimum TTL across all records. With the current implementation, if all records have TTL values greater than maxTTL, minTTL will incorrectly remain at maxTTL instead of the actual minimum.

Suggested change

let minTTL = this.#maxTTL

let minTTL = Infinity

Copilot · 2025-09-26T19:42:44Z

docs/docs/api/Dispatcher.md

+  get(origin) {
+    return lru.get(origin);
+  },
+  set(origin, records, { ttl }) {


The destructuring assignment assumes the options parameter will always be passed and contain a ttl property. This should be updated to handle cases where options might be undefined: set(origin, records, options = {}) { const { ttl } = options; ... }

Suggested change

set(origin, records, { ttl }) {

set(origin, records, options = {}) {

const { ttl } = options;

codecov-commenter · 2025-09-26T19:55:26Z

Codecov Report

❌ Patch coverage is 98.18182% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.94%. Comparing base (d23bf9c) to head (47d5739).
⚠️ Report is 18 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/interceptor/dns.js	98.18%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           main    #4589       +/-   ##
=========================================
+ Coverage      0   92.94%   +92.94%     
=========================================
  Files         0      106      +106     
  Lines         0    33015    +33015     
=========================================
+ Hits          0    30685    +30685     
- Misses        0     2330     +2330

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…erage

metcoder95 · 2025-09-30T06:57:29Z

docs/docs/api/Dispatcher.md

  - The function should return a single record from the records array.
  - By default a simplified version of Round Robin is used.
  - The `records` property can be mutated to store the state of the balancing algorithm.
+- `storage: { size: number, get(origin: string): DNSInterceptorOriginRecords | null, set(origin: string, records: DNSInterceptorOriginRecords | null, options?: { ttl: number }): void, delete(origin: string): void, full(): boolean }` - Custom storage for resolved DNS records


Let's decompose it into its own section within the DNS interceptor itself

metcoder95 · 2025-09-30T07:00:42Z

lib/interceptor/dns.js

      if (typeof record.ttl === 'number') {
        // The record TTL is expected to be in ms
        record.ttl = Math.min(record.ttl, this.#maxTTL)
+        minTTL = Math.min(minTTL, record.ttl)


It should be fine to keep it as is, this just duplicates it.

metcoder95 · 2025-09-30T07:00:52Z

lib/interceptor/dns.js


-    this.#records.set(origin.hostname, records)
+    // We provide a default TTL if external storage will be used without TTL per record-level support
+    this.storage.set(origin.hostname, records, { ttl: minTTL })


The third argument is not used isn't it?

mcollina

lgtm

mcollina

Can you add test for the types?

SuperOleg39 · 2025-10-06T11:31:12Z

Can you add test for the types?

Yep, ready

SuperOleg39 and others added 3 commits September 22, 2025 14:27

feat(interceptors/dns): cache option

d35fb9e

feat(interceptors/dns): extract storage

2c156e1

fix(interceptors/dns): correct lookup types

128125f

SuperOleg39 commented Sep 26, 2025

View reviewed changes

o.drapeza added 3 commits September 26, 2025 16:39

fix(interceptors/dns): storage parameter validation

667f956

test(interceptors/dns): test case for external storage

3689532

docs(interceptors/dns): example with LRU cache as a storage

5b1d923

SuperOleg39 mentioned this pull request Sep 26, 2025

feat(interceptors/dns): cache option #4565

Closed

7 tasks

SuperOleg39 commented Sep 26, 2025

View reviewed changes

Uzlopak requested a review from Copilot September 26, 2025 19:41

Copilot AI reviewed Sep 26, 2025

View reviewed changes

test(interceptors/dns): test storage option validation for better cov…

5a1fc07

…erage

metcoder95 reviewed Sep 30, 2025

View reviewed changes

docs(interceptors/dns): separate DNSStorage interface documentation

238f0b3

mcollina approved these changes Oct 4, 2025

View reviewed changes

mcollina reviewed Oct 4, 2025

View reviewed changes

test(interceptors/dns): test types

47d5739

metcoder95 approved these changes Oct 7, 2025

View reviewed changes

metcoder95 merged commit 83db0fc into nodejs:main Oct 8, 2025
28 of 29 checks passed

-    this.storage.set(origin.hostname, records, { ttl: minTTL })
+    if (typeof this.storage.set === 'function' && this.storage.set.length >= 3) {
+      this.storage.set(origin.hostname, records, { ttl: minTTL })
+    } else {
+      this.storage.set(origin.hostname, records)
+    }

	set(origin, records, { ttl }) {
	set(origin, records, options = {}) {
	const { ttl } = options;

Uh oh!

Feat dns interceptor storage #4589

Feat dns interceptor storage #4589

Uh oh!

Conversation

SuperOleg39 commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale

Changes

Status

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuperOleg39 Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

SuperOleg39 commented Oct 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

SuperOleg39 commented Sep 26, 2025 •

edited

Loading

SuperOleg39 Sep 26, 2025 •

edited

Loading

codecov-commenter commented Sep 26, 2025 •

edited

Loading