Skip to content

Commit

Permalink
Implementing review suggestions on cacheDnsTtl.
Browse files Browse the repository at this point in the history
  • Loading branch information
marciopd committed Mar 19, 2020
1 parent b315cf8 commit bf639b1
Show file tree
Hide file tree
Showing 5 changed files with 10 additions and 9 deletions.
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@ Parameters (specified as one object passed into hot-shots):
* `prefix`: What to prefix each stat name with `default: ''`
* `suffix`: What to suffix each stat name with `default: ''`
* `globalize`: Expose this StatsD instance globally. `default: false`
* `cacheDns`: Caches dns lookup to *host* for *dnsTtlMilliseconds*, only used
* `cacheDns`: Caches dns lookup to *host* for *cacheDnsTtl*, only used
when protocol is `udp`, `default: false`
* `dnsTtlMilliseconds`: time-to-live of dns lookups in milliseconds, when *cacheDns* is enabled. `default: 60000`
* `cacheDnsTtl`: time-to-live of dns lookups in milliseconds, when *cacheDns* is enabled. `default: 60000`
* `mock`: Create a mock StatsD instance, sending no stats to
the server and allowing data to be read from mockBuffer. Note that
mockBuffer will keep growing, so only use for testing or clear out periodically. `default: false`
Expand Down
5 changes: 3 additions & 2 deletions lib/statsd.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const { PROTOCOL } = require('./constants');
const createTransport = require('./transport');

const UDS_DEFAULT_GRACEFUL_RESTART_LIMIT = 1000;
const CACHE_DNS_TTL_DEFAULT = 60000;

/**
* The Client for StatsD. The main entry-point for hot-shots. Note adding new parameters
Expand Down Expand Up @@ -44,7 +45,7 @@ const Client = function (host, port, prefix, suffix, globalize, cacheDns, mock,
this.protocol = PROTOCOL.UDP;
}
this.cacheDns = options.cacheDns === true;
this.dnsTtlMilliseconds = options.dnsTtlMilliseconds || 60000;
this.cacheDnsTtl = options.cacheDnsTtl || CACHE_DNS_TTL_DEFAULT;
this.host = options.host || process.env.DD_AGENT_HOST || 'localhost';
this.port = options.port || parseInt(process.env.DD_DOGSTATSD_PORT, 10) || 8125;
this.prefix = options.prefix || '';
Expand Down Expand Up @@ -93,7 +94,7 @@ const Client = function (host, port, prefix, suffix, globalize, cacheDns, mock,
this.socket = createTransport(this, {
host: this.host,
cacheDns: this.cacheDns,
dnsTtlMilliseconds: this.dnsTtlMilliseconds,
cacheDnsTtl: this.cacheDnsTtl,
path: options.path,
port: this.port,
protocol: this.protocol,
Expand Down
2 changes: 1 addition & 1 deletion lib/transport.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ const createUdpTransport = args => {

const sendUsingDnsCache = (callback, buf) => {
const now = new Date();

This comment has been minimized.

Copy link
@STRML

STRML Mar 30, 2020

@marciopd Date.now() will be faster than instantiating a whole Date object

This comment has been minimized.

Copy link
@bdeitte

bdeitte Mar 30, 2020

Collaborator

Good call, please feel free to create a PR for this, or anyone else reading this!

This comment has been minimized.

Copy link
@marciopd

marciopd Mar 30, 2020

Author Contributor

Hi @STRML and @bdeitte . Sure, I can open a PR for this.

This comment has been minimized.

Copy link
@marciopd

marciopd Mar 30, 2020

Author Contributor

PR: #160

if (dnsResolutionData.resolvedAddress === undefined || (now - dnsResolutionData.timestamp > args.dnsTtlMilliseconds)) {
if (dnsResolutionData.resolvedAddress === undefined || (now - dnsResolutionData.timestamp > args.cacheDnsTtl)) {
dns.lookup(args.host, (error, address) => {
if (error) {
callback(error);
Expand Down
6 changes: 3 additions & 3 deletions test/udpDnsCacheTransport.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,10 @@ describe('#udpDnsCacheTransport', () => {
server = createServer(udpServerType, opts => {
const socketMock = mockDgramSocket();

const dnsTtlMilliseconds = 100;
const cacheDnsTtl = 100;
statsd = createHotShotsClient(Object.assign(opts, {
cacheDns: true,
dnsTtlMilliseconds: dnsTtlMilliseconds
cacheDnsTtl: cacheDnsTtl
}), 'client');

const resolvedHostAddress = '1.1.1.1';
Expand All @@ -151,7 +151,7 @@ describe('#udpDnsCacheTransport', () => {
done();
}, 1000);
});
}, dnsTtlMilliseconds + 1);
}, cacheDnsTtl + 1);
});
});
});
Expand Down
2 changes: 1 addition & 1 deletion types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ declare module "hot-shots" {
bufferFlushInterval?: number;
bufferHolder?: { buffer: string };
cacheDns?: boolean;
dnsTtlMilliseconds?: number;
cacheDnsTtl?: number;
errorHandler?: (err: Error) => void;
globalTags?: Tags;
globalize?: boolean;
Expand Down

0 comments on commit bf639b1

Please sign in to comment.