Skip to content

remove async storage from http2 server instrumentation #5947

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

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
34 changes: 14 additions & 20 deletions packages/datadog-instrumentations/src/http2/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,13 @@

const {
channel,
addHook,
AsyncResource
addHook
} = require('../helpers/instrument')
const shimmer = require('../../../datadog-shimmer')

const startServerCh = channel('apm:http2:server:request:start')
const errorServerCh = channel('apm:http2:server:request:error')
const finishServerCh = channel('apm:http2:server:request:finish')
const emitCh = channel('apm:http2:server:response:emit')

const names = ['http2', 'node:http2']

Expand All @@ -30,18 +29,14 @@ function wrapCreateServer (createServer) {
}
}

function wrapResponseEmit (emit) {
const asyncResource = new AsyncResource('bound-anonymous-fn')
function wrapResponseEmit (emit, ctx) {
return function (eventName, event) {
return asyncResource.runInAsyncScope(() => {
if (eventName === 'close' && finishServerCh.hasSubscribers) {
finishServerCh.publish({ req: this.req })
}

return emit.apply(this, arguments)
})
ctx.req = this.req
ctx.eventName = eventName
return emitCh.runStores(ctx, emit, this, ...arguments)
}
}

function wrapEmit (emit) {
return function (eventName, req, res) {
if (!startServerCh.hasSubscribers) {
Expand All @@ -51,18 +46,17 @@ function wrapEmit (emit) {
if (eventName === 'request') {
res.req = req

const asyncResource = new AsyncResource('bound-anonymous-fn')
return asyncResource.runInAsyncScope(() => {
startServerCh.publish({ req, res })

shimmer.wrap(res, 'emit', wrapResponseEmit)
const ctx = { req, res }
return startServerCh.runStores(ctx, () => {
shimmer.wrap(res, 'emit', emit => wrapResponseEmit(emit, ctx))

try {
return emit.apply(this, arguments)
} catch (err) {
errorServerCh.publish(err)
} catch (error) {
ctx.error = error
errorServerCh.publish(ctx)

throw err
throw error
}
})
}
Expand Down
6 changes: 6 additions & 0 deletions packages/datadog-plugin-child_process/test/index.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ describe('Child process plugin', () => {
expect(tracerStub.startSpan).to.have.been.calledOnceWithExactly(
'command_execution',
{
startTime: undefined,
childOf: undefined,
tags: {
component: 'subprocess',
Expand All @@ -75,6 +76,7 @@ describe('Child process plugin', () => {
expect(tracerStub.startSpan).to.have.been.calledOnceWithExactly(
'command_execution',
{
startTime: undefined,
childOf: undefined,
tags: {
component: 'subprocess',
Expand All @@ -100,6 +102,7 @@ describe('Child process plugin', () => {
expect(tracerStub.startSpan).to.have.been.calledOnceWithExactly(
'command_execution',
{
startTime: undefined,
childOf: undefined,
tags: {
component: 'subprocess',
Expand All @@ -126,6 +129,7 @@ describe('Child process plugin', () => {
expect(tracerStub.startSpan).to.have.been.calledOnceWithExactly(
'command_execution',
{
startTime: undefined,
childOf: undefined,
tags: {
component: 'subprocess',
Expand Down Expand Up @@ -153,6 +157,7 @@ describe('Child process plugin', () => {
expect(tracerStub.startSpan).to.have.been.calledOnceWithExactly(
'command_execution',
{
startTime: undefined,
childOf: undefined,
tags: {
component: 'subprocess',
Expand Down Expand Up @@ -180,6 +185,7 @@ describe('Child process plugin', () => {
expect(tracerStub.startSpan).to.have.been.calledOnceWithExactly(
'command_execution',
{
startTime: undefined,
childOf: undefined,
tags: {
component: 'subprocess',
Expand Down
8 changes: 4 additions & 4 deletions packages/datadog-plugin-http/src/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,16 @@ class HttpServerPlugin extends ServerPlugin {
return 'http'
}

static get prefix () {
return 'apm:http:server:request'
}

constructor (...args) {
super(...args)
this._parentStore = undefined
this.addTraceSub('exit', message => this.exit(message))
}

addTraceSub (eventName, handler) {
this.addSub(`apm:${this.constructor.id}:server:${this.operation}:${eventName}`, handler)
}

start ({ req, res, abortController }) {
const store = storage('legacy').getStore()
const span = web.startSpan(
Expand Down
31 changes: 23 additions & 8 deletions packages/datadog-plugin-http2/src/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,26 @@
// Plugin temporarily disabled. See https://github.com/DataDog/dd-trace-js/issues/312

const ServerPlugin = require('../../dd-trace/src/plugins/server')
const { storage } = require('../../datadog-core')
const web = require('../../dd-trace/src/plugins/util/web')
const { COMPONENT } = require('../../dd-trace/src/constants')

class Http2ServerPlugin extends ServerPlugin {
constructor (tracer, config) {
super(tracer, config)
this.addBind('apm:http2:server:response:emit', this.bindEmit)
}

static get id () {
return 'http2'
}

addTraceSub (eventName, handler) {
this.addSub(`apm:${this.constructor.id}:server:${this.operation}:${eventName}`, handler)
static get prefix () {
return 'apm:http2:server:request'
}

start ({ req, res }) {
const store = storage('legacy').getStore()
bindStart (ctx) {
const { req, res } = ctx

const span = web.startSpan(
this.tracer,
{
Expand All @@ -26,28 +31,38 @@ class Http2ServerPlugin extends ServerPlugin {
},
req,
res,
this.operationName()
this.operationName(),
ctx
)

span.setTag(COMPONENT, this.constructor.id)
span._integrationName = this.constructor.id

this.enter(span, { ...store, req, res })
ctx.currentStore.req = req
ctx.currentStore.res = res

const context = web.getContext(req)

if (!context.instrumented) {
context.res.writeHead = web.wrapWriteHead(context)
context.instrumented = true
}

return ctx.currentStore
}

finish ({ req }) {
bindEmit (ctx) {
if (ctx.eventName !== 'close') return ctx.currentStore

const { req } = ctx

const context = web.getContext(req)

if (!context || !context.res) return // Not created by a http.Server instance.

web.finishAll(context)

return ctx.currentStore
}

error (error) {
Expand Down
2 changes: 1 addition & 1 deletion packages/dd-trace/src/plugins/plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ module.exports = class Plugin {
}

get tracer () {
return this._tracer._tracer
return this._tracer?._tracer || this._tracer
}

enter (span, store) {
Expand Down
24 changes: 19 additions & 5 deletions packages/dd-trace/src/plugins/tracing.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,24 +102,38 @@ class TracingPlugin extends Plugin {
}
}

startSpan (name, { childOf, kind, meta, metrics, service, resource, type } = {}, enterOrCtx = true) {
startSpan (name, options = {}, enterOrCtx = true) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
startSpan (name, options = {}, enterOrCtx = true) {
startSpan (name, options = {}, enterOrCtx = true) {
let { childOf, kind, meta, metrics, service, resource, type } = options

It's best to not alter object received as a parameter to a function. This also means restoring direct usage below.

let {
component = this.component,
childOf,
integrationName,
kind,
meta,
metrics,
service,
startTime,
resource,
type
} = options

const store = storage('legacy').getStore()
if (store && childOf === undefined) {
childOf = store.span
}

const span = this.tracer.startSpan(name, {
const span = this._tracer.startSpan(name, {
startTime,
childOf,
tags: {
[COMPONENT]: this.component,
'service.name': service || this.tracer._service,
[COMPONENT]: component,
'service.name': service || meta?.service || this._tracer._service,
'resource.name': resource,
'span.kind': kind,
'span.type': type,
...meta,
...metrics
},
integrationName: this.component,
integrationName: integrationName || component,
links: childOf?._links
})

Expand Down
34 changes: 15 additions & 19 deletions packages/dd-trace/src/plugins/util/inferred_proxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const supportedProxies = {
}
}

function createInferredProxySpan (headers, childOf, tracer, context) {
function createInferredProxySpan (headers, childOf, tracer, reqCtx, traceCtx, startSpanHelper) {
if (!headers) {
return null
}
Expand All @@ -41,26 +41,22 @@ function createInferredProxySpan (headers, childOf, tracer, context) {

log.debug('Successfully extracted inferred span info %s for proxy:', proxyContext, proxyContext.proxySystemName)

const span = tracer.startSpan(
proxySpanInfo.spanName,
{
childOf,
type: 'web',
startTime: proxyContext.requestTime,
integrationName: proxySpanInfo.component,
tags: {
service: proxyContext.domainName || tracer._config.service,
component: proxySpanInfo.component,
[SPAN_TYPE]: 'web',
[HTTP_METHOD]: proxyContext.method,
[HTTP_URL]: proxyContext.domainName + proxyContext.path,
stage: proxyContext.stage
}
const span = startSpanHelper(tracer, proxySpanInfo.spanName, {
childOf,
type: 'web',
startTime: proxyContext.requestTime,
integrationName: proxySpanInfo.component,
meta: {
service: proxyContext.domainName || tracer._config.service,
component: proxySpanInfo.component,
[SPAN_TYPE]: 'web',
[HTTP_METHOD]: proxyContext.method,
[HTTP_URL]: proxyContext.domainName + proxyContext.path,
stage: proxyContext.stage
}
)
}, traceCtx)

tracer.scope().activate(span)
context.inferredProxySpan = span
reqCtx.inferredProxySpan = span
childOf = span

log.debug('Successfully created inferred proxy span.')
Expand Down
Loading
Loading