Skip to content

Commit e6e966d

Browse files
watsonwatson
authored
refactor(exporters): split AgentInfoExporter into focused modules (#7310)
The AgentInfoExporter class had mixed responsibilities, serving as both an agent info fetcher and a trace buffering exporter. This made the code confusing and led to unnecessary instantiations. Changes: - Split agent info fetching into agent/info.js with fetchAgentInfo utility - Extract buffering logic into BufferingExporter base class - Update CiVisibilityExporter to extend BufferingExporter - Update openfeature and llmobs writers to use fetchAgentInfo directly This improves code clarity by adhering to single responsibility principle and reduces overhead by avoiding unnecessary class instantiations when only agent info is needed.
1 parent ab24010 commit e6e966d

File tree

13 files changed

+319
-147
lines changed

13 files changed

+319
-147
lines changed

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828
"test:debugger": "mocha packages/dd-trace/test/debugger/**/*.spec.js",
2929
"test:debugger:ci": "nyc --no-clean --include \"packages/dd-trace/src/debugger/**/*.js\" -- npm run test:debugger",
3030
"test:eslint-rules": "node eslint-rules/*.test.mjs",
31-
"test:trace:core": "node scripts/mocha-parallel-files.js --expose-gc --timeout 30000 -- \"packages/dd-trace/test/*.spec.js\" \"packages/dd-trace/test/{ci-visibility,config,crashtracking,datastreams,encode,exporters,msgpack,opentelemetry,opentracing,payload-tagging,plugins,remote_config,service-naming,standalone,telemetry,external-logger}/**/*.spec.js\"",
31+
"test:trace:core": "node scripts/mocha-parallel-files.js --expose-gc --timeout 30000 -- \"packages/dd-trace/test/*.spec.js\" \"packages/dd-trace/test/{agent,ci-visibility,config,crashtracking,datastreams,encode,exporters,msgpack,opentelemetry,opentracing,payload-tagging,plugins,remote_config,service-naming,standalone,telemetry,external-logger}/**/*.spec.js\"",
3232
"test:trace:core:ci": "nyc --no-clean --include \"packages/dd-trace/src/**/*.js\" -- npm run test:trace:core",
3333
"test:trace:guardrails": "mocha packages/dd-trace/test/guardrails/**/*.spec.js",
3434
"test:trace:guardrails:ci": "nyc --no-clean --include \"packages/dd-trace/src/guardrails/**/*.js\" -- npm run test:trace:guardrails",

‎packages/dd-trace/src/agent/info.js‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
'use strict'
2+
3+
const request = require('../exporters/common/request')
4+
5+
/**
6+
* Fetches agent information from the /info endpoint
7+
* @param {URL} url - The agent URL
8+
* @param {Function} callback - Callback function with signature (err, agentInfo)
9+
*/
10+
function fetchAgentInfo (url, callback) {
11+
request('', {
12+
path: '/info',
13+
url
14+
}, (err, res) => {
15+
if (err) {
16+
return callback(err)
17+
}
18+
try {
19+
const response = JSON.parse(res)
20+
return callback(null, response)
21+
} catch (e) {
22+
return callback(e)
23+
}
24+
})
25+
}
26+
27+
module.exports = { fetchAgentInfo }

‎packages/dd-trace/src/agent/url.js‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
'use strict'
2+
3+
const { URL, format } = require('url')
4+
const defaults = require('../config/defaults')
5+
6+
module.exports = { getAgentUrl }
7+
8+
// TODO: Investigate merging with the getAgentUrl function in config/index.js which has
9+
// additional logic for unix socket auto-detection on Linux. The config version is only used
10+
// during config initialization, while this one is used throughout the codebase. Consider if
11+
// the unix socket detection should be part of this general helper or remain config-specific.
12+
13+
/**
14+
* Gets the agent URL from config, constructing it from hostname/port if needed
15+
* @param {ReturnType<import('../config')>} config - Tracer configuration object
16+
* @returns {URL} The agent URL
17+
*/
18+
function getAgentUrl (config) {
19+
const { url, hostname = defaults.hostname, port = defaults.port } = config
20+
return url || new URL(format({
21+
protocol: 'http:',
22+
hostname,
23+
port
24+
}))
25+
}

‎packages/dd-trace/src/ci-visibility/exporters/agent-proxy/index.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ const AgentWriter = require('../../../exporters/agent/writer')
44
const AgentlessWriter = require('../agentless/writer')
55
const CoverageWriter = require('../agentless/coverage-writer')
66
const CiVisibilityExporter = require('../ci-visibility-exporter')
7+
const { fetchAgentInfo } = require('../../../agent/info')
78

89
const AGENT_EVP_PROXY_PATH_PREFIX = '/evp_proxy/v'
910
const AGENT_EVP_PROXY_PATH_REGEX = /\/evp_proxy\/v(\d+)\/?/
@@ -42,7 +43,7 @@ class AgentProxyCiVisibilityExporter extends CiVisibilityExporter {
4243
isTestDynamicInstrumentationEnabled
4344
} = config
4445

45-
this.getAgentInfo((err, agentInfo) => {
46+
fetchAgentInfo(this._url, (err, agentInfo) => {
4647
this._isInitialized = true
4748
let latestEvpProxyVersion = getLatestEvpProxyVersion(err, agentInfo)
4849
const isEvpCompatible = latestEvpProxyVersion >= 2

‎packages/dd-trace/src/ci-visibility/exporters/ci-visibility-exporter.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ const { getKnownTests: getKnownTestsRequest } = require('../early-flake-detectio
88
const { getTestManagementTests: getTestManagementTestsRequest } =
99
require('../test-management/get-test-management-tests')
1010
const log = require('../../log')
11-
const AgentInfoExporter = require('../../exporters/common/agent-info-exporter')
11+
const BufferingExporter = require('../../exporters/common/buffering-exporter')
1212
const { GIT_REPOSITORY_URL, GIT_COMMIT_SHA } = require('../../plugins/util/tags')
1313
const { sendGitMetadata: sendGitMetadataRequest } = require('./git/git_metadata')
1414

@@ -34,7 +34,7 @@ function getIsTestSessionTrace (trace) {
3434
const GIT_UPLOAD_TIMEOUT = 60_000 // 60 seconds
3535
const CAN_USE_CI_VIS_PROTOCOL_TIMEOUT = GIT_UPLOAD_TIMEOUT
3636

37-
class CiVisibilityExporter extends AgentInfoExporter {
37+
class CiVisibilityExporter extends BufferingExporter {
3838
constructor (config) {
3939
super(config)
4040
this._timer = undefined

packages/dd-trace/src/exporters/common/agent-info-exporter.js renamed to packages/dd-trace/src/exporters/common/buffering-exporter.js

Lines changed: 10 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -1,47 +1,20 @@
11
'use strict'
22

3-
const { URL, format } = require('url')
4-
5-
const defaults = require('../../config/defaults')
63
const { incrementCountMetric, TELEMETRY_EVENTS_ENQUEUED_FOR_SERIALIZATION } = require('../../ci-visibility/telemetry')
7-
const request = require('./request')
8-
9-
function fetchAgentInfo (url, callback) {
10-
request('', {
11-
path: '/info',
12-
url
13-
}, (err, res) => {
14-
if (err) {
15-
return callback(err)
16-
}
17-
try {
18-
const response = JSON.parse(res)
19-
return callback(null, response)
20-
} catch (e) {
21-
return callback(e)
22-
}
23-
})
24-
}
4+
const { getAgentUrl } = require('../../agent/url')
255

266
/**
27-
* Exporter that exposes a way to query /info endpoint from the agent and gives you the response.
28-
* While this._writer is not initialized, exported traces are stored as is.
7+
* Base exporter that buffers traces until a writer is initialized.
8+
* Provides common export logic with flush intervals.
299
*/
30-
class AgentInfoExporter {
10+
class BufferingExporter {
11+
_traceBuffer = []
12+
_isInitialized = false
13+
_writer
14+
3115
constructor (tracerConfig) {
3216
this._config = tracerConfig
33-
const { url, hostname = defaults.hostname, port } = this._config
34-
this._url = url || new URL(format({
35-
protocol: 'http:',
36-
hostname,
37-
port
38-
}))
39-
this._traceBuffer = []
40-
this._isInitialized = false
41-
}
42-
43-
getAgentInfo (onReceivedInfo) {
44-
fetchAgentInfo(this._url, onReceivedInfo)
17+
this._url = getAgentUrl(tracerConfig)
4518
}
4619

4720
export (trace) {
@@ -86,4 +59,4 @@ class AgentInfoExporter {
8659
}
8760
}
8861

89-
module.exports = AgentInfoExporter
62+
module.exports = BufferingExporter

‎packages/dd-trace/src/llmobs/writers/util.js‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,8 @@
33
const logger = require('../../log')
44
const { EVP_PROXY_AGENT_BASE_PATH } = require('../constants/writers')
55
const telemetry = require('../telemetry')
6-
7-
const AgentInfoExporter = require('../../exporters/common/agent-info-exporter')
8-
/** @type {AgentInfoExporter} */
9-
let agentInfoExporter
6+
const { fetchAgentInfo } = require('../../agent/info')
7+
const { getAgentUrl } = require('../../agent/url')
108

119
function setAgentStrategy (config, setWritersAgentlessValue) {
1210
const agentlessEnabled = config.llmobs.agentlessEnabled
@@ -16,11 +14,7 @@ function setAgentStrategy (config, setWritersAgentlessValue) {
1614
return
1715
}
1816

19-
if (!agentInfoExporter) {
20-
agentInfoExporter = new AgentInfoExporter(config)
21-
}
22-
23-
agentInfoExporter.getAgentInfo((err, agentInfo) => {
17+
fetchAgentInfo(getAgentUrl(config), (err, agentInfo) => {
2418
if (err) {
2519
setWritersAgentlessValue(true)
2620
return

‎packages/dd-trace/src/openfeature/writers/util.js‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,21 +2,16 @@
22

33
const logger = require('../../log')
44
const { EVP_PROXY_AGENT_BASE_PATH } = require('../constants/constants')
5-
6-
const AgentInfoExporter = require('../../exporters/common/agent-info-exporter')
7-
let agentInfoExporter
5+
const { fetchAgentInfo } = require('../../agent/info')
6+
const { getAgentUrl } = require('../../agent/url')
87

98
/**
109
* Determines if the agent supports EVP proxy and sets the writer enabled state accordingly
1110
* @param {import('../../config')} config - Tracer configuration object
1211
* @param {Function} setWriterEnabledValue - Callback to set the writer enabled state
1312
*/
1413
function setAgentStrategy (config, setWriterEnabledValue) {
15-
if (!agentInfoExporter) {
16-
agentInfoExporter = new AgentInfoExporter(config)
17-
}
18-
19-
agentInfoExporter.getAgentInfo((err, agentInfo) => {
14+
fetchAgentInfo(getAgentUrl(config), (err, agentInfo) => {
2015
if (err) {
2116
logger.debug('FFE Writer disabled - error getting agent info:', err.message)
2217
setWriterEnabledValue(false)

‎packages/dd-trace/test/agent/info.spec.js‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
'use strict'
2+
3+
const assert = require('node:assert/strict')
4+
5+
const { describe, it } = require('mocha')
6+
const nock = require('nock')
7+
8+
require('../setup/core')
9+
const { fetchAgentInfo } = require('../../src/agent/info')
10+
11+
describe('agent/info', () => {
12+
const port = 8126
13+
const url = `http://127.0.0.1:${port}`
14+
15+
describe('fetchAgentInfo', () => {
16+
it('should query /info endpoint and parse response', (done) => {
17+
const scope = nock(url)
18+
.get('/info')
19+
.reply(200, JSON.stringify({
20+
endpoints: ['/evp_proxy/v2']
21+
}))
22+
23+
assert.notStrictEqual(scope.isDone(), true)
24+
fetchAgentInfo(new URL(url), (err, response) => {
25+
assert.strictEqual(err, null)
26+
assert.deepStrictEqual(response.endpoints, ['/evp_proxy/v2'])
27+
assert.strictEqual(scope.isDone(), true)
28+
done()
29+
})
30+
})
31+
32+
it('should handle error responses', (done) => {
33+
const scope = nock(url)
34+
.get('/info')
35+
.reply(500, 'Internal Server Error')
36+
37+
fetchAgentInfo(new URL(url), (err, response) => {
38+
assert.ok(err)
39+
assert.strictEqual(response, undefined)
40+
assert.strictEqual(scope.isDone(), true)
41+
done()
42+
})
43+
})
44+
45+
it('should handle invalid JSON responses', (done) => {
46+
const scope = nock(url)
47+
.get('/info')
48+
.reply(200, 'invalid json')
49+
50+
fetchAgentInfo(new URL(url), (err, response) => {
51+
assert.ok(err)
52+
assert.ok(err instanceof SyntaxError)
53+
assert.strictEqual(response, undefined)
54+
assert.strictEqual(scope.isDone(), true)
55+
done()
56+
})
57+
})
58+
})
59+
})

‎packages/dd-trace/test/agent/url.spec.js‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
'use strict'
2+
3+
const assert = require('node:assert/strict')
4+
const { URL } = require('url')
5+
6+
const { describe, it } = require('mocha')
7+
8+
require('../setup/core')
9+
const { getAgentUrl } = require('../../src/agent/url')
10+
const defaults = require('../../src/config/defaults')
11+
12+
describe('agent/url', () => {
13+
describe('getAgentUrl', () => {
14+
it('should return the url from config when provided', () => {
15+
const url = new URL('http://custom-host:9999')
16+
const config = { url }
17+
18+
const result = getAgentUrl(config)
19+
20+
assert.strictEqual(result, url)
21+
})
22+
23+
it('should construct URL from hostname and port', () => {
24+
const config = {
25+
hostname: 'custom-host',
26+
port: '9999'
27+
}
28+
29+
const result = getAgentUrl(config)
30+
31+
assert.ok(result instanceof URL)
32+
assert.strictEqual(result.hostname, 'custom-host')
33+
assert.strictEqual(result.port, '9999')
34+
assert.strictEqual(result.protocol, 'http:')
35+
})
36+
37+
it('should use default hostname when not provided', () => {
38+
const config = {
39+
port: '9999'
40+
}
41+
42+
const result = getAgentUrl(config)
43+
44+
assert.strictEqual(result.hostname, defaults.hostname)
45+
assert.strictEqual(result.port, '9999')
46+
})
47+
48+
it('should use default port when not provided in config', () => {
49+
const config = {
50+
hostname: 'custom-host'
51+
}
52+
53+
const result = getAgentUrl(config)
54+
55+
assert.strictEqual(result.hostname, 'custom-host')
56+
assert.strictEqual(result.port, defaults.port)
57+
assert.strictEqual(result.protocol, 'http:')
58+
})
59+
60+
it('should use defaults when hostname and port not provided', () => {
61+
const config = {}
62+
63+
const result = getAgentUrl(config)
64+
65+
assert.strictEqual(result.hostname, defaults.hostname)
66+
assert.strictEqual(result.port, defaults.port)
67+
assert.strictEqual(result.protocol, 'http:')
68+
})
69+
70+
it('should prioritize url over hostname and port', () => {
71+
const url = new URL('http://url-host:1111')
72+
const config = {
73+
url,
74+
hostname: 'ignored-host',
75+
port: '2222'
76+
}
77+
78+
const result = getAgentUrl(config)
79+
80+
assert.strictEqual(result, url)
81+
assert.strictEqual(result.hostname, 'url-host')
82+
assert.strictEqual(result.port, '1111')
83+
})
84+
85+
it('should support IPv6 hostnames', () => {
86+
const config = {
87+
hostname: '::1',
88+
port: '8126'
89+
}
90+
91+
const result = getAgentUrl(config)
92+
93+
// IPv6 addresses get wrapped in brackets by URL constructor
94+
assert.strictEqual(result.hostname, '[::1]')
95+
assert.strictEqual(result.port, '8126')
96+
assert.ok(result.href.includes('[::1]:8126'))
97+
})
98+
})
99+
})

0 commit comments

Comments
 (0)