Hapi 17 #146
Hapi 17 #146
Conversation
Qard
force-pushed
the
hapi-17
branch
3 times, most recently
from
212476c
to
ecf2f0e
Compare
Codecov Report
@@ Coverage Diff @@
## master #146 +/- ##
==========================================
- Coverage 79.52% 79.14% -0.38%
==========================================
Files 39 39
Lines 1939 1947 +8
==========================================
- Hits 1542 1541 -1
- Misses 397 406 +9
Continue to review full report at Codecov.
|
|
@watson Looks like it's working here. I'll get it cleaned up tomorrow and perhaps you can review it when you're back to work? |
Qard
force-pushed
the
hapi-17
branch
2 times, most recently
from
692dbd4
to
ee81389
Compare
lib/instrumentation/modules/hapi.js
Outdated
| // When the hapi server has no connections we don't make connection | ||
| // lifecycle hooks | ||
| var conns = server.connections | ||
| if (conns && conns.length === 0) { |
There was a problem hiding this comment.
This needs an hapi 17 check since in hapi 17 connections are not a thing any more.
There was a problem hiding this comment.
The existence check of server.connections covers that.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We'd never reach that debug message on hapi 17, because server.connections would have to exist.
There was a problem hiding this comment.
it was more for correctness ^^
There was a problem hiding this comment.
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)
lib/instrumentation/modules/hapi.js
Outdated
| return | ||
| } | ||
|
|
||
| // Hooks that are only allowed when the hapi server has connections |
There was a problem hiding this comment.
Not a 100% correct statement anymore
There was a problem hiding this comment.
The check is still fine afaik
There was a problem hiding this comment.
@watson Any thoughts on wording here?
There was a problem hiding this comment.
@AdriVanHoudt Do you mean related to the removal of multi-connections support in hapi 17? What would be a more appropriate comment?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@AdriVanHoudt how about just Hooks that are only allowed when the hapi server has connections (with hapi 17+ this is always the case)
Qard
force-pushed
the
hapi-17
branch
2 times, most recently
from
c5c672e
to
8b68ed7
Compare
This adds hapi v17 support.