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.

Sign up for GitHub

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

update analytics config with a simpler API that also fixes plugin config #573

Merged
merged 2 commits into from
May 29, 2019
Merged

update analytics config with a simpler API that also fixes plugin config #573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7b23207
update analytics config with a simpler API that also fixes plugin config
rochdev May 8, 2019
77b4223
use the new config and remove old plugin config
rochdev May 22, 2019
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
31 changes: 13 additions & 18 deletions src/analytics_sampler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,27 +2,22 @@

const ANALYTICS = require('../ext/tags').ANALYTICS

module.exports = {
sample (span, config, useDefault) {
if (!config || config.enabled !== true) return
let enabled = false

if (useDefault) {
if (config.sampleRate === undefined) {
span.setTag(ANALYTICS, 1)
} else if (config.sampleRate >= 0 && config.sampleRate <= 1) {
span.setTag(ANALYTICS, config.sampleRate)
}
}
module.exports = {
enable () {
enabled = true
},

if (config.sampleRates && typeof config.sampleRates === 'object') {
const name = span.context()._name
disable () {
enabled = false
},

if (config.sampleRates.hasOwnProperty(name)) {
this.sample(span, {
enabled: true,
sampleRate: config.sampleRates[name]
}, true)
}
sample (span, rate, inherit) {
if (rate !== undefined) {
span.setTag(ANALYTICS, rate)
} else if (inherit && enabled) {
span.setTag(ANALYTICS, 1)
}
}
}
26 changes: 13 additions & 13 deletions src/format.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ function format (span) {
extractError(formatted, span)
extractTags(formatted, span)
extractMetrics(formatted, span)
extractAnalytics(formatted, span)

return formatted
}
Expand Down Expand Up @@ -99,8 +100,6 @@ function extractError (trace, span) {
function extractMetrics (trace, span) {
const spanContext = span.context()

let analytics = spanContext._tags[ANALYTICS]

Object.keys(spanContext._metrics).forEach(metric => {
if (typeof spanContext._metrics[metric] === 'number') {
trace.metrics[metric] = spanContext._metrics[metric]
Expand All @@ -110,18 +109,19 @@ function extractMetrics (trace, span) {
if (spanContext._sampling.priority !== undefined) {
trace.metrics[SAMPLING_PRIORITY_KEY] = spanContext._sampling.priority
}
}

switch (typeof analytics) {
case 'string':
analytics = parseFloat(analytics)
case 'number': // eslint-disable-line no-fallthrough
if (!isNaN(analytics)) {
trace.metrics[ANALYTICS_KEY] = Math.max(Math.min(analytics, 1), 0)
}
break
case 'boolean':
trace.metrics[ANALYTICS_KEY] = analytics ? 1 : 0
break
function extractAnalytics (trace, span) {
let analytics = span.context()._tags[ANALYTICS]

if (analytics === true) {
analytics = 1
} else {
analytics = parseFloat(analytics)
}

if (!isNaN(analytics)) {
trace.metrics[ANALYTICS_KEY] = Math.max(Math.min(analytics, 1), 0)
}
}

Expand Down
19 changes: 0 additions & 19 deletions src/instrumenter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,14 +155,6 @@ class Instrumenter {
}

_set (plugin, meta) {
const analytics = {}

if (typeof this._tracer._tracer._analytics === 'boolean') {
analytics.enabled = this._tracer._tracer._analytics
}

meta.config.analytics = Object.assign(analytics, normalizeAnalyticsConfig(meta.config.analytics))

this._plugins.set(plugin, meta)
this._load(plugin, meta)
}
Expand Down Expand Up @@ -241,17 +233,6 @@ class Instrumenter {
}
}

function normalizeAnalyticsConfig (config) {
switch (typeof config) {
case 'boolean':
return { enabled: config }
case 'object':
if (config) return config
default: // eslint-disable-line no-fallthrough
return {}
}
}

function getModules (instrumentation) {
const modules = []
const ids = Object.keys(require.cache)
Expand Down
4 changes: 1 addition & 3 deletions src/plugins/graphql.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ function startExecutionSpan (tracer, config, operation, args) {
addDocumentTags(span, args.document)
addVariableTags(tracer, config, span, args.variableValues)

analyticsSampler.sample(span, config.analytics, true)
analyticsSampler.sample(span, config.analytics)

return span
}
Expand Down Expand Up @@ -307,8 +307,6 @@ function startResolveSpan (tracer, config, childOf, path, info, contextValue) {
}
}

analyticsSampler.sample(span, config.analytics)

return span
}

Expand Down
2 changes: 0 additions & 2 deletions src/plugins/util/web.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,6 @@ const web = {
[RESOURCE_NAME]: middleware._name || middleware.name || '<anonymous>'
})

analyticsSampler.sample(span, req._datadog.config.analytics)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we not sampling this anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

Without the per span name configuration this would never be sampled, so it was simply removed.


req._datadog.middleware.push(span)

return tracer.scope().activate(span, fn)
Expand Down
5 changes: 5 additions & 0 deletions src/proxy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const Config = require('./config')
const Instrumenter = require('./instrumenter')
const platform = require('./platform')
const log = require('./log')
const analyticsSampler = require('./analytics_sampler')

const noop = new NoopTracer()

Expand Down Expand Up @@ -36,6 +37,10 @@ class Tracer extends BaseTracer {
platform.metrics().start()
}

if (config.analytics) {
analyticsSampler.enable()
}

this._tracer = new DatadogTracer(config)
this._instrumenter.enable()
this._instrumenter.patch(config)
Expand Down
62 changes: 11 additions & 51 deletions test/analytics_sampler.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,69 +17,29 @@ describe('analyticsSampler', () => {
})

describe('sample', () => {
it('should use a sample rate of 1 by default', () => {
sampler.sample(span, {
enabled: true
}, true)
it('should sample a span', () => {
sampler.sample(span, true)

expect(span.setTag).to.have.been.calledWith(ANALYTICS, 1)
expect(span.setTag).to.have.been.calledWith(ANALYTICS, true)
})

it('should sample a span with the provided rate', () => {
sampler.sample(span, {
enabled: true,
sampleRate: 0.5
}, true)

expect(span.setTag).to.have.been.calledWith(ANALYTICS, 0.5)
})

it('should sample only when enabled', () => {
sampler.sample(span, {
sampleRate: 0.5
}, true)

expect(span.setTag).to.not.have.been.called
})
sampler.sample(span, 0)

it('should sample only with the flag to use the default', () => {
sampler.sample(span, {
enabled: true,
sampleRate: 0.5
})

expect(span.setTag).to.not.have.been.called
expect(span.setTag).to.have.been.calledWith(ANALYTICS, 0)
})

it('should sample a span with the operation specific rate', () => {
sampler.sample(span, {
enabled: true,
sampleRates: {
'web.request': 0.5
}
})

expect(span.setTag).to.have.been.calledWith(ANALYTICS, 0.5)
})

it('should ignore invalid values', () => {
sampler.sample(span)
sampler.sample(span, 2)
sampler.sample(span, -1)
sampler.sample(span, 'foo')
it('should not set a rate by default', () => {
sampler.sample(span, undefined)

expect(span.setTag).to.not.have.been.called
})

it('should ignore rates for different operation names', () => {
sampler.sample(span, {
enabled: true,
sampleRates: {
'other.request': 0.5
}
})
it('should inherit from global setting when unset', () => {
sampler.enable()
sampler.sample(span, undefined, true)

expect(span.setTag).to.not.have.been.called
expect(span.setTag).to.have.been.calledWith(ANALYTICS, 1)
})
})
})
2 changes: 1 addition & 1 deletion test/format.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ describe('format', () => {
it('should accept boolean false for analytics', () => {
spanContext._tags[ANALYTICS] = false
trace = format(span)
expect(trace.metrics[ANALYTICS_KEY]).to.equal(0)
expect(trace.metrics[ANALYTICS_KEY]).to.be.undefined
})

it('should accept strings for analytics', () => {
Expand Down
72 changes: 0 additions & 72 deletions test/instrumenter.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,78 +166,6 @@ describe('Instrumenter', () => {
expect(integrations.express.patch).to.have.been.calledOnce
})

it('should have a default for analytics', () => {
instrumenter.use('express-mock')

const express = require('express-mock')

expect(integrations.express.patch).to.have.been.calledWith(express, 'tracer', {
analytics: {}
})
})

it('should support analytics configuration on the tracer', () => {
const analytics = {
enabled: true
}

tracer._tracer = { _analytics: true }

instrumenter.use('express-mock')

const express = require('express-mock')

expect(integrations.express.patch).to.have.been.calledWithMatch(express, tracer._tracer, {
analytics
})
})

it('should support analytics configuration on the plugin', () => {
const analytics = {
enabled: true,
sampleRate: 0.5,
sampleRates: {
'express.request': 0.1
}
}

instrumenter.use('express-mock', { analytics })

const express = require('express-mock')

expect(integrations.express.patch).to.have.been.calledWithMatch(express, 'tracer', {
analytics
})
})

it('should prioritize the analytics configuration from the plugin', () => {
const analytics = {
enabled: true
}

tracer._tracer = {
_analytics: false
}

instrumenter.use('express-mock', { analytics })

const express = require('express-mock')

expect(integrations.express.patch).to.have.been.calledWithMatch(express, tracer._tracer, {
analytics
})
})

it('should support shorthand syntax to enable analytics', () => {
instrumenter.use('express-mock', { analytics: true })

const express = require('express-mock')

expect(integrations.express.patch).to.have.been.calledWithMatch(express, tracer._tracer, {
analytics: { enabled: true }
})
})

it('should not patch disabled plugins', () => {
instrumenter.use('express-mock', { enabled: false })

Expand Down
16 changes: 15 additions & 1 deletion test/proxy.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ describe('TracerProxy', () => {
let Config
let config
let platform
let analyticsSampler

beforeEach(() => {
tracer = {
Expand Down Expand Up @@ -56,12 +57,17 @@ describe('TracerProxy', () => {
})
}

analyticsSampler = {
enable: sinon.spy()
}

Proxy = proxyquire('../src/proxy', {
'./tracer': DatadogTracer,
'./noop/tracer': NoopTracer,
'./instrumenter': Instrumenter,
'./config': Config,
'./platform': platform
'./platform': platform,
'./analytics_sampler': analyticsSampler
})

proxy = new Proxy()
Expand Down Expand Up @@ -132,6 +138,14 @@ describe('TracerProxy', () => {

expect(platform.metrics().start).to.have.been.called
})

it('should enable the analytics sampler when configured', () => {
config.analytics = true

proxy.init()

expect(analyticsSampler.enable).to.have.been.called
})
})

describe('trace', () => {
Expand Down