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

Hapi 17 #146

Merged
merged 3 commits into from Jan 3, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion .tav.yml
Expand Up @@ -66,6 +66,12 @@ elasticsearch:
handlebars:
versions: '*'
commands: node test/instrumentation/modules/handlebars.js
hapi:
hapi-no-async-await:
name: hapi
versions: '>=9.0.1 <17.0.0'
commands: node test/instrumentation/modules/hapi.js
hapi-async-await:
name: hapi
node: '>=8.2'
versions: '^17.0.0'
commands: node test/instrumentation/modules/hapi.js
221 changes: 128 additions & 93 deletions lib/instrumentation/modules/hapi.js
Expand Up @@ -14,113 +14,148 @@ module.exports = function (hapi, agent, version) {

debug('shimming hapi.Server.prototype.initialize')

shimmer.wrap(hapi.Server.prototype, 'initialize', function (orig) {
return function () {
// Hooks that are always allowed
if (typeof this.on === 'function') {
this.on('request-error', function (req, err) {
agent.captureError(err, {request: req.raw && req.raw.req})
})
if (semver.satisfies(version, '>=17')) {
shimmer.massWrap(hapi, ['Server', 'server'], function (orig) {
return function (options) {
var res = orig.apply(this, arguments)
patchServer(res)
return res
}
})
} else {
shimmer.wrap(hapi.Server.prototype, 'initialize', function (orig) {
return function () {
patchServer(this)
return orig.apply(this, arguments)
}
})
}

this.on('log', function (event, tags) {
if (!event || !tags.error) return
function patchServer (server) {
// Hooks that are always allowed
if (typeof server.on === 'function') {
attachEvents(server)
} else if (typeof server.events.on === 'function') {
attachEvents(server.events)
} else {
debug('unable to enable hapi error tracking')
}

var payload = {custom: event} // TODO: Find better location to put this than custom
// Prior to hapi 17, when the server has no connections we can't make
// connection lifecycle hooks (in hapi 17+ the server always has
// connections, though the `server.connections` property doesn't exists,
// so this if-statement wont fire)
var conns = server.connections
if (conns && conns.length === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs an hapi 17 check since in hapi 17 connections are not a thing any more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The existence check of server.connections covers that.

Copy link
Member

Choose a reason for hiding this comment

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

I guess what @AdriVanHoudt means is that outputting the debug message "unable to enable hapi instrumentation on connectionless server" isn't really that nice if running hapi 17?

So it should be more like if (!hapi17plus && conns && cons.length === 0) { where hapi17plus is a boolean that's true only of we're running hapi version 17 or higher.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'd never reach that debug message on hapi 17, because server.connections would have to exist.

Copy link
Member

Choose a reason for hiding this comment

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

Ah my bad 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

it was more for correctness ^^

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I see it can be a little confusing to read because of the above comment. How about just updating the comment to something like:

// Prior to hapi 17, when the server has no connections we can't make connection
// lifecycle hooks (in hapi 17+ the server always have connections, though the
// `server.connections` property doesn't exists, so this if-statement wont fire)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

debug('unable to enable hapi instrumentation on connectionless server')
return
}

var err = event.data
if (!(err instanceof Error) && typeof err !== 'string') {
err = 'hapi server emitted a log event tagged error'
}
// Hooks that are only allowed when the hapi server has connections
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a 100% correct statement anymore

Copy link
Contributor

Choose a reason for hiding this comment

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

The check is still fine afaik

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@watson Any thoughts on wording here?

Copy link
Member

@watson watson Jan 2, 2018

Choose a reason for hiding this comment

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

@AdriVanHoudt Do you mean related to the removal of multi-connections support in hapi 17? What would be a more appropriate comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can tell hapi 17 will have it always.
In 16 it only becomes available after adding a connecting but in 17 you can't run a server without it. The moment you create the server the ext will be there in 17.
So maybe When we can hook into the lifecycle to do proper transaction naming or something?

Copy link
Member

Choose a reason for hiding this comment

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

@AdriVanHoudt how about just Hooks that are only allowed when the hapi server has connections (with hapi 17+ this is always the case)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

// (with hapi 17+ this is always the case)
if (typeof server.ext === 'function') {
server.ext('onPreAuth', onPreAuth)
server.ext('onPreResponse', onPreResponse)
} else {
debug('unable to enable automatic hapi transaction naming')
}
}

agent.captureError(err, payload)
function attachEvents (emitter) {
if (semver.satisfies(version, '<17')) {
emitter.on('request-error', function (request, error) {
agent.captureError(error, {
request: request.raw && request.raw.req
})
})
}

this.on('request', function (req, event, tags) {
if (!event || !tags.error) return

var payload = {
custom: event, // TODO: Find better location to put this than custom
request: req.raw && req.raw.req
}
emitter.on('log', function (event, tags) {
captureError('log', null, event, tags)
})

var err = event.data
if (!(err instanceof Error) && typeof err !== 'string') {
err = 'hapi server emitted a request event tagged error'
}
emitter.on('request', function (req, event, tags) {
captureError('request', req, event, tags)
})
}

agent.captureError(err, payload)
})
} else {
debug('unable to enable hapi error tracking')
}
function captureError (type, req, event, tags) {
if (!event || !tags.error) return

// TODO: Find better location to put this than custom
var payload = {
custom: {
tags: event.tags,
internals: event.internals,
// Moved from data to error in hapi 17
data: event.data || event.error
},
request: req && req.raw && req.raw.req
}

// When the hapi server has no connections we don't make connection
// lifecycle hooks
if (this.connections.length === 0) {
debug('unable to enable hapi instrumentation on connectionless server')
return orig.apply(this, arguments)
}
var err = payload.custom.data
if (!(err instanceof Error) && typeof err !== 'string') {
err = 'hapi server emitted a ' + type + ' event tagged error'
}

// Hooks that are only allowed when the hapi server has connections
if (typeof this.ext === 'function') {
this.ext('onPreAuth', function (request, reply) {
debug('received hapi onPreAuth event')

// Record the fact that the preAuth extension have been called. This
// info is useful later to know if this is a CORS preflight request
// that is automatically handled by hapi (as those will not trigger
// the onPreAuth extention)
request._elastic_apm_onPreAuth = true

if (request.route) {
// fingerprint was introduced in hapi 11 and is a little more
// stable in case the param names change
// - path example: /foo/{bar*2}
// - fingerprint example: /foo/?/?
var fingerprint = request.route.fingerprint || request.route.path

if (fingerprint) {
var name = (request.raw && request.raw.req && request.raw.req.method) ||
(request.route.method && request.route.method.toUpperCase())

if (typeof name === 'string') {
name = name + ' ' + fingerprint
} else {
name = fingerprint
}

agent._instrumentation.setDefaultTransactionName(name)
}
}

return reply.continue()
})
agent.captureError(err, payload)
}

this.ext('onPreResponse', function (request, reply) {
debug('received hapi onPreResponse event')

// Detection of CORS preflight requests:
// There is no easy way in hapi to get the matched route for a
// CORS preflight request that matches any of the autogenerated
// routes created by hapi when `cors: true`. The best solution is to
// detect the request "fingerprint" using the magic if-sentence below
// and group all those requests into on type of transaction
if (!request._elastic_apm_onPreAuth &&
request.route && request.route.path === '/{p*}' &&
request.raw && request.raw.req && request.raw.req.method === 'OPTIONS' &&
request.raw.req.headers['access-control-request-method']) {
agent._instrumentation.setDefaultTransactionName('CORS preflight')
}

return reply.continue()
})
} else {
debug('unable to enable automatic hapi transaction naming')
function onPreAuth (request, reply) {
debug('received hapi onPreAuth event')

// Record the fact that the preAuth extension have been called. This
// info is useful later to know if this is a CORS preflight request
// that is automatically handled by hapi (as those will not trigger
// the onPreAuth extention)
request._elastic_apm_onPreAuth = true

if (request.route) {
// fingerprint was introduced in hapi 11 and is a little more
// stable in case the param names change
// - path example: /foo/{bar*2}
// - fingerprint example: /foo/?/?
var fingerprint = request.route.fingerprint || request.route.path

if (fingerprint) {
var name = (request.raw && request.raw.req && request.raw.req.method) ||
(request.route.method && request.route.method.toUpperCase())

if (typeof name === 'string') {
name = name + ' ' + fingerprint
} else {
name = fingerprint
}

agent._instrumentation.setDefaultTransactionName(name)
}
}

return semver.satisfies(version, '>=17')
? reply.continue
: reply.continue()
}

return orig.apply(this, arguments)
function onPreResponse (request, reply) {
debug('received hapi onPreResponse event')

// Detection of CORS preflight requests:
// There is no easy way in hapi to get the matched route for a
// CORS preflight request that matches any of the autogenerated
// routes created by hapi when `cors: true`. The best solution is to
// detect the request "fingerprint" using the magic if-sentence below
// and group all those requests into on type of transaction
if (!request._elastic_apm_onPreAuth &&
request.route && request.route.path === '/{p*}' &&
request.raw && request.raw.req && request.raw.req.method === 'OPTIONS' &&
request.raw.req.headers['access-control-request-method']) {
agent._instrumentation.setDefaultTransactionName('CORS preflight')
}
})

return semver.satisfies(version, '>=17')
? reply.continue
: reply.continue()
}

return hapi
}
2 changes: 1 addition & 1 deletion package.json
Expand Up @@ -95,7 +95,7 @@
"get-port": "^2.1.0",
"graphql": "^0.11.2",
"handlebars": "^4.0.11",
"hapi": "^16.5.2",
"hapi": "^16.6.2",
"https-pem": "^2.0.0",
"inquirer": "^0.12.0",
"ioredis": "^3.0.0",
Expand Down