Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add protocol as a configuration option for the dd-trace-agent URL #416

Merged
merged 6 commits into from
Jan 24, 2019
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/API.md
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,7 @@ Options can be configured as a parameter to the [init()](https://datadog.github.
| enabled | DD_TRACE_ENABLED | true | Whether to enable the tracer. |
| debug | DD_TRACE_DEBUG | false | Enable debug logging in the tracer. |
| service | DD_SERVICE_NAME | | The service name to be used for this program. |
| overrideUrl | DD_TRACE_AGENT_URL | | The url of the trace agent that the tracer will submit to. Takes priority over hostname and port, if set. |
| hostname | DD_TRACE_AGENT_HOSTNAME | localhost | The address of the trace agent that the tracer will submit to. |
| port | DD_TRACE_AGENT_PORT | 8126 | The port of the trace agent that the tracer will submit to. |
| env | DD_ENV | | Set an application’s environment e.g. `prod`, `pre-prod`, `stage`. |
Expand Down
3 changes: 2 additions & 1 deletion src/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ class Config {
const enabled = coalesce(options.enabled, platform.env('DD_TRACE_ENABLED'), true)
const debug = coalesce(options.debug, platform.env('DD_TRACE_DEBUG'), false)
const env = coalesce(options.env, platform.env('DD_ENV'))
const overrideUrl = coalesce(options.overrideUrl, platform.env('DD_TRACE_AGENT_URL'), null)
const protocol = 'http'
const hostname = coalesce(
options.hostname,
Expand All @@ -26,7 +27,7 @@ class Config {
this.enabled = String(enabled) === 'true'
this.debug = String(debug) === 'true'
this.env = env
this.url = new URL(`${protocol}://${hostname}:${port}`)
this.url = overrideUrl ? new URL(overrideUrl) : new URL(`${protocol}://${hostname}:${port}`)
this.tags = Object.assign({}, options.tags)
this.flushInterval = flushInterval
this.bufferSize = 100000
Expand Down
4 changes: 3 additions & 1 deletion src/platform/node/request.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
'use strict'

const http = require('http')
const https = require('https')

function request (options, callback) {
options = Object.assign({
Expand All @@ -14,7 +15,8 @@ function request (options, callback) {
options.headers['Content-Length'] = byteLength(data)

return new Promise((resolve, reject) => {
const req = http.request(options, res => {
const client = options.protocol === 'https:' ? https : http
const req = client.request(options, res => {
let data = ''

res.on('data', chunk => { data += chunk })
Expand Down
2 changes: 2 additions & 0 deletions src/proxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ class Tracer extends BaseTracer {
* @param {boolean} [options.enabled=true] Whether to enable the tracer.
* @param {boolean} [options.debug=false] Enable debug logging in the tracer.
* @param {string} [options.service] The service name to be used for this program.
* @param {string} [options.overrideUrl=null] The url to the trace agent that the tracer will submit to. Takes
* precedence over hostname and port, if set.
* @param {string} [options.hostname=localhost] The address of the trace agent that the tracer will submit to.
* @param {number|string} [options.port=8126] The port of the trace agent that the tracer will submit to.
* @param {number} [options.sampleRate=1] Percentage of spans to sample as a float between 0 and 1.
Expand Down
90 changes: 88 additions & 2 deletions test/config.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,33 @@ describe('Config', () => {

expect(config).to.have.property('enabled', false)
expect(config).to.have.property('debug', true)
expect(config).to.have.nested.property('url.protocol', 'http:')
expect(config).to.have.nested.property('url.hostname', 'agent')
expect(config).to.have.nested.property('url.port', '6218')
expect(config).to.have.property('service', 'service')
expect(config).to.have.property('env', 'test')
})

it('should initialize from environment variables with url taking precedence', () => {
platform.env.withArgs('DD_TRACE_AGENT_URL').returns('https://agent2:7777')
platform.env.withArgs('DD_TRACE_AGENT_HOSTNAME').returns('agent')
platform.env.withArgs('DD_TRACE_AGENT_PORT').returns('6218')
platform.env.withArgs('DD_TRACE_ENABLED').returns('false')
platform.env.withArgs('DD_TRACE_DEBUG').returns('true')
platform.env.withArgs('DD_SERVICE_NAME').returns('service')
platform.env.withArgs('DD_ENV').returns('test')

const config = new Config()

expect(config).to.have.property('enabled', false)
expect(config).to.have.property('debug', true)
expect(config).to.have.nested.property('url.protocol', 'https:')
expect(config).to.have.nested.property('url.hostname', 'agent2')
expect(config).to.have.nested.property('url.port', '7777')
expect(config).to.have.property('service', 'service')
expect(config).to.have.property('env', 'test')
})

it('should initialize from the options', () => {
const logger = {}
const tags = { foo: 'bar' }
Expand All @@ -74,6 +95,7 @@ describe('Config', () => {

expect(config).to.have.property('enabled', false)
expect(config).to.have.property('debug', true)
expect(config).to.have.nested.property('url.protocol', 'http:')
expect(config).to.have.nested.property('url.hostname', 'agent')
expect(config).to.have.nested.property('url.port', '6218')
expect(config).to.have.property('service', 'service')
Expand All @@ -85,6 +107,38 @@ describe('Config', () => {
expect(config).to.have.property('plugins', false)
})

it('should initialize from the options with overrideUrl taking precedence', () => {
const logger = {}
const tags = { foo: 'bar' }
const config = new Config({
enabled: false,
debug: true,
hostname: 'agent',
overrideUrl: 'https://agent2:7777',
Copy link
Member

Choose a reason for hiding this comment

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

Why not just url or endpoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, i thought it might be a little confusing with just url, but the update docs should clear that up incase its not clear

port: 6218,
service: 'service',
env: 'test',
sampleRate: 0.5,
logger,
tags,
flushInterval: 5000,
plugins: false
})

expect(config).to.have.property('enabled', false)
expect(config).to.have.property('debug', true)
expect(config).to.have.nested.property('url.protocol', 'https:')
expect(config).to.have.nested.property('url.hostname', 'agent2')
expect(config).to.have.nested.property('url.port', '7777')
expect(config).to.have.property('service', 'service')
expect(config).to.have.property('env', 'test')
expect(config).to.have.property('sampleRate', 0.5)
expect(config).to.have.property('logger', logger)
expect(config).to.have.deep.property('tags', tags)
expect(config).to.have.property('flushInterval', 5000)
expect(config).to.have.property('plugins', false)
})

it('should give priority to the common agent environment variable', () => {
platform.env.withArgs('DD_TRACE_AGENT_HOSTNAME').returns('trace-agent')
platform.env.withArgs('DD_AGENT_HOST').returns('agent')
Expand All @@ -95,6 +149,7 @@ describe('Config', () => {
})

it('should give priority to the options', () => {
platform.env.withArgs('DD_TRACE_AGENT_URL').returns('https://agent2:6218')
platform.env.withArgs('DD_TRACE_AGENT_HOSTNAME').returns('agent')
platform.env.withArgs('DD_TRACE_AGENT_PORT').returns('6218')
platform.env.withArgs('DD_TRACE_ENABLED').returns('false')
Expand All @@ -105,6 +160,7 @@ describe('Config', () => {
const config = new Config({
enabled: true,
debug: false,
protocol: 'https',
hostname: 'server',
port: 7777,
service: 'test',
Expand All @@ -113,8 +169,38 @@ describe('Config', () => {

expect(config).to.have.property('enabled', true)
expect(config).to.have.property('debug', false)
expect(config).to.have.nested.property('url.hostname', 'server')
expect(config).to.have.nested.property('url.port', '7777')
expect(config).to.have.nested.property('url.protocol', 'https:')
expect(config).to.have.nested.property('url.hostname', 'agent2')
expect(config).to.have.nested.property('url.port', '6218')
expect(config).to.have.property('service', 'test')
expect(config).to.have.property('env', 'development')
})

it('should give priority to the options especially overrideUrl', () => {
platform.env.withArgs('DD_TRACE_AGENT_URL').returns('http://agent2:6218')
platform.env.withArgs('DD_TRACE_AGENT_HOSTNAME').returns('agent')
platform.env.withArgs('DD_TRACE_AGENT_PORT').returns('6218')
platform.env.withArgs('DD_TRACE_ENABLED').returns('false')
platform.env.withArgs('DD_TRACE_DEBUG').returns('true')
platform.env.withArgs('DD_SERVICE_NAME').returns('service')
platform.env.withArgs('DD_ENV').returns('test')

const config = new Config({
enabled: true,
debug: false,
overrideUrl: 'https://agent3:7778',
protocol: 'http',
hostname: 'server',
port: 7777,
service: 'test',
env: 'development'
})

expect(config).to.have.property('enabled', true)
expect(config).to.have.property('debug', false)
expect(config).to.have.nested.property('url.protocol', 'https:')
expect(config).to.have.nested.property('url.hostname', 'agent3')
expect(config).to.have.nested.property('url.port', '7778')
expect(config).to.have.property('service', 'test')
expect(config).to.have.property('env', 'development')
})
Expand Down