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

feat: support node 16; also test with node 15, drop testing of node 13 #2055

Merged
merged 18 commits into from
Apr 30, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
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
3 changes: 2 additions & 1 deletion .ci/.jenkins_nightly_nodejs.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
NODEJS_VERSION:
- "16"
- "15"
- "14"
- "13"
- "12"
- "10"
4 changes: 3 additions & 1 deletion .ci/.jenkins_nodejs.yml
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
NODEJS_VERSION:
- "16"
- "16.0"
- "15"
- "14"
- "14.0"
- "13"
- "12"
- "12.0"
- "10"
Expand Down
3 changes: 2 additions & 1 deletion .ci/.jenkins_rc_nodejs.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
NODEJS_VERSION:
- "16"
- "15"
- "14"
- "13"
- "12"
- "10"
3 changes: 2 additions & 1 deletion .ci/.jenkins_tav_nodejs.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
NODEJS_VERSION:
- "16"
- "15"
- "14"
- "13"
- "12"
- "10"
- "8"
10 changes: 10 additions & 0 deletions .ci/scripts/docker-test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,16 @@ if [[ $major_node_version -eq 8 ]] && [[ $minor_node_version -lt 8 ]]; then
export NODE_OPTIONS="${NODE_OPTIONS:+${NODE_OPTIONS}} --expose-http2"
fi

# "test/instrumentation/modules/http2.js" fails if the OpenSSL SECLEVEL=2,
# which is the case in the node:16 Docker image and could be in other
# environments. Here we explicitly set it to SECLEVEL=0 for testing.
#
# Skip for node v8 because it results in this warning:
# openssl config failed: error:25066067:DSO support routines:DLFCN_LOAD:could not load the shared library
if [[ $major_node_version -gt 8 ]]; then
export NODE_OPTIONS="${NODE_OPTIONS:+${NODE_OPTIONS}} --openssl-config=./test/openssl-config-for-testing.cnf"
fi

# Workaround to git <2.7
# error fatal: unable to look up current user in the passwd file: no such user
# see http://git.661346.n2.nabble.com/git-clone-fails-when-current-user-is-not-in-etc-passwd-td7643604.html
Expand Down
4 changes: 3 additions & 1 deletion .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,11 @@ jobs:
strategy:
matrix:
node:
- '16'
- '16.0'
- '15'
- '14'
- '14.0'
- '13'
- '12'
- '12.0'
- '10'
Expand Down
9 changes: 9 additions & 0 deletions .npmrc
Original file line number Diff line number Diff line change
@@ -1 +1,10 @@
package-lock=false

# Workaround unresolvable peerDependencies between express-graphql, graphql,
# and apollo-server-express. npm v7 (included with node v15) makes these
# peerDependencies issues an install error. Until the community catches up
# and resolves peerDependencies issues or apm-agent-nodejs.git's tests are
# setup to not have competing deps in "devDependencies", we revert to the
# pre-v7 behavior.
# https://docs.npmjs.com/cli/v7/using-npm/config#legacy-peer-deps
legacy-peer-deps=true
23 changes: 17 additions & 6 deletions .tav.yml
Original file line number Diff line number Diff line change
Expand Up @@ -285,35 +285,46 @@ hapi-v9-v15:
node: '>=4 <14'
commands:
- node test/instrumentation/modules/hapi/basic-legacy-path.js
- node test/instrumentation/modules/hapi/set-framework.js
- node test/instrumentation/modules/hapi/set-framework-hapi.js
hapi-v16:
name: hapi
versions: '>=16.0.0 <17.0.0'
node: '>=4'
commands:
- node test/instrumentation/modules/hapi/basic-legacy-path.js
- node test/instrumentation/modules/hapi/set-framework.js
- node test/instrumentation/modules/hapi/set-framework-hapi.js
hapi-prenodev15:
name: hapi
versions: '>=17.0.0'
node: '>=8.12.0 <15.0.0'
commands:
- node test/instrumentation/modules/hapi/basic-legacy-path.js
- node test/instrumentation/modules/hapi/set-framework-hapi.js
hapi:
name: hapi
# Work around https://github.com/npm/cli/issues/2267 in npm@7.
# Note: An alternative might be to just not test the "hapi" package with
# node >= 15, given that "hapi" was deprecated before node v16.
preinstall: rm -rf node_modules/hapi
node: '>=15.0.0'
versions: '>=17.0.0'
node: '>=8.12.0'
commands:
- node test/instrumentation/modules/hapi/basic-legacy-path.js
- node test/instrumentation/modules/hapi/set-framework.js
- node test/instrumentation/modules/hapi/set-framework-hapi.js
'@hapi/hapi-v17-v18':
name: '@hapi/hapi'
versions: '>=17.0.0 <19.0.0'
node: '>=8.12.0'
commands:
- node test/instrumentation/modules/hapi/basic.js
- node test/instrumentation/modules/hapi/set-framework-2.js
- node test/instrumentation/modules/hapi/set-framework-hapihapi.js
'@hapi/hapi':
name: '@hapi/hapi'
versions: '>=19.0.0'
node: '>=12'
commands:
- node test/instrumentation/modules/hapi/basic.js
- node test/instrumentation/modules/hapi/set-framework-2.js
- node test/instrumentation/modules/hapi/set-framework-hapihapi.js

tedious:
name: tedious
Expand Down
15 changes: 15 additions & 0 deletions CHANGELOG.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,21 @@ Notes:
[[release-notes-3.x]]
=== Node.js Agent version 3.x

==== Unreleased

[float]
===== Breaking changes

[float]
===== Features

* Add support for Node.js v16. (This also drops testing of Node.js v13
releases.)

[float]
===== Bug fixes


[[release-notes-3.14.0]]
==== 3.14.0 - 2021/04/19

Expand Down
8 changes: 4 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
"url": "git://github.com/elastic/apm-agent-nodejs.git"
},
"engines": {
"node": "^8.6.0 || 10 || 12 || 13 || 14"
"node": "^8.6.0 || 10 || 12 || 13 || 14 || 15 || 16"
},
"keywords": [
"opbeat",
Expand Down Expand Up @@ -115,17 +115,16 @@
"@babel/core": "^7.8.4",
"@babel/preset-env": "^7.8.4",
"@elastic/elasticsearch": "^7.10.0",
"body-parser": "^1.19.0",
"@hapi/hapi": "^18.4.1",
"@hapi/hapi": "^20.1.2",
Copy link
Member Author

Choose a reason for hiding this comment

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

The "POST" handler test in test/instrumentation/modules/hapi.js hangs with @hapi/[email protected] and node 16. Let's just move to testing with the latest hapi (which does mean only testing with node v12 and later). The TAV tests cover the rest.

"@koa/router": "^9.0.1",
"koa-bodyparser": "^3.2.0",
"@types/node": "^13.7.4",
"ajv": "^6.12.6",
"apollo-server-express": "^2.10.1",
"aws-sdk": "^2.622.0",
"backport": "^5.1.2",
"benchmark": "^2.1.4",
"bluebird": "^3.7.2",
"body-parser": "^1.19.0",
"cassandra-driver": "^4.4.0",
"clone": "^2.0.0",
"columnify": "^1.5.4",
Expand All @@ -151,6 +150,7 @@
"jade": "^1.11.0",
"knex": "^0.21.2",
"koa": "^2.11.0",
"koa-bodyparser": "^3.2.0",
"koa-router": "^9.0.1",
"lambda-local": "^1.7.1",
"memcached": "^2.2.2",
Expand Down
32 changes: 32 additions & 0 deletions test/_is_hapi_incompat.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
'use strict'

var semver = require('semver')

// 'hapi' and '@hapi/hapi' versions have some challenges with compat with
// various versions of node. This method tells you if the current versions
// are incompatible.
function isHapiIncompat (moduleName) {
var hapiVersion = require(`${moduleName}/package.json`).version

// hapi 17+ requires Node.js 8.9.0 or higher
if (semver.lt(process.version, '8.9.0') && semver.gte(hapiVersion, '17.0.0')) {
return true
}
// hapi 19+ requires Node.js 12 or higher
if (semver.lt(process.version, '12.0.0') && semver.gte(hapiVersion, '19.0.0')) {
return true
}

// hapi does not work on early versions of Node.js 10 because of
// https://github.com/nodejs/node/issues/20516
//
// NOTE: Do not use semver.satisfies, as it does not match prereleases
var parsed = semver.parse(process.version)
if (parsed.major === 10 && parsed.minor >= 0 && parsed.minor < 8) {
return true
}

return false
}

module.exports = isHapiIncompat
6 changes: 3 additions & 3 deletions test/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ var config = require('../lib/config')
var Instrumentation = require('../lib/instrumentation')
var apmVersion = require('../package').version
var apmName = require('../package').name
var isHapiIncompat = require('./_is_hapi_incompat')

process.env.ELASTIC_APM_METRICS_INTERVAL = '0'
process.env.ELASTIC_APM_CENTRAL_CONFIG = 'false'
Expand Down Expand Up @@ -710,16 +711,15 @@ usePathAsTransactionNameTests.forEach(function (usePathAsTransactionNameTest) {
})

test('disableInstrumentations', function (t) {
var hapiVersion = require('hapi/package.json').version
var expressGraphqlVersion = require('express-graphql/package.json').version
var esVersion = require('@elastic/elasticsearch/package.json').version

var flattenedModules = Instrumentation.modules.reduce((acc, val) => acc.concat(val), [])
var modules = new Set(flattenedModules)
if (semver.lt(process.version, '8.9.0') && semver.gte(hapiVersion, '17.0.0')) {
if (isHapiIncompat('hapi')) {
modules.delete('hapi')
}
if (semver.lt(process.version, '8.9.0')) {
if (isHapiIncompat('@hapi/hapi')) {
modules.delete('@hapi/hapi')
}
if (semver.lt(process.version, '7.6.0') && semver.gte(expressGraphqlVersion, '0.9.0')) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ const agent = require('../../../..').start({
centralConfig: false
})

const pkg = require('hapi/package')
const semver = require('semver')

// hapi 17+ requires Node.js 8.9.0 or higher
if (semver.lt(process.version, '8.9.0') && semver.gte(pkg.version, '17.0.0')) process.exit()
var isHapiIncompat = require('../../../_is_hapi_incompat')
if (isHapiIncompat('hapi')) {
// Skip out of this test.
process.exit()
}

let asserts = 0

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@ const agent = require('../../../..').start({
metricsInterval: 0
})

const pkg = require('@hapi/hapi/package')
const semver = require('semver')

// hapi 17+ requires Node.js 8.9.0 or higher
if (semver.lt(process.version, '8.9.0') && semver.gte(pkg.version, '17.0.0')) process.exit()
var isHapiIncompat = require('../../../_is_hapi_incompat')
if (isHapiIncompat('@hapi/hapi')) {
// Skip out of this test.
process.exit()
}

let asserts = 0

Expand Down
16 changes: 7 additions & 9 deletions test/instrumentation/modules/hapi/shared.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,18 @@ module.exports = (moduleName) => {
metricsInterval: 0,
centralConfig: false
})
var pkg = require(`${moduleName}/package.json`)
var semver = require('semver')

// hapi 17+ requires Node.js 8.9.0 or higher
if (semver.lt(process.version, '8.9.0') && semver.gte(pkg.version, '17.0.0')) process.exit()

// hapi does not work on early versions of Node.js 10 because of https://github.com/nodejs/node/issues/20516
// NOTE: Do not use semver.satisfies, as it does not match prereleases
var parsed = semver.parse(process.version)
if (parsed.major === 10 && parsed.minor >= 0 && parsed.minor < 8) process.exit()
var isHapiIncompat = require('../../../_is_hapi_incompat')
if (isHapiIncompat(moduleName)) {
// Skip out of this test.
process.exit()
}

var http = require('http')

var Hapi = require(moduleName)
var pkg = require(moduleName + '/package.json')
var semver = require('semver')
var test = require('tape')

var mockClient = require('../../../_mock_http_client')
Expand Down
23 changes: 22 additions & 1 deletion test/instrumentation/modules/http/aborted-requests-enabled.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,23 @@ test('client-side abort below error threshold - call end', { timeout: 10000 }, f

var server = http.createServer(function (req, res) {
setTimeout(function () {
// Explicitly respond with headers before aborting the client request,
// because:
// (a) `assert(t, data)` above asserts that `trans.result` has been set
// to "HTTP 2xx", which depends on the wrapped `writeHead` having been
// called, and
// (b) calling res.write('...') or res.end('...') *after* a clientReq.abort()
// in node >=15 leads to a race on whether `ServerResponse.writeHead()`
// is called.
//
// The race:
// - clientReq.abort() closes the client-side of the socket
// - The server-side of the socket closes (`onClose` in lib/_http_agent.js)
// - (race) If the server-side socket is closed before `res.write` is
// called, then res.writeHead() will not be called as of this change:
// https://github.com/nodejs/node/pull/31818/files#diff-48d21edbddb6e855d1ee5716c49bcdc0d913c11ee8a24a98ea7dbc60cd253556L661-R706
res.writeHead(200)

clientReq.abort()
res.write('sync write')
process.nextTick(function () {
Expand Down Expand Up @@ -80,9 +97,11 @@ test('client-side abort above error threshold - call end', function (t) {

var server = http.createServer(function (req, res) {
setTimeout(function () {
res.writeHead(200) // See race comment above.

clientReq.abort()
setTimeout(function () {
res.write('Hello') // server emits clientError if written in same tick as abort
res.write('Hello')
setTimeout(function () {
res.end(' World')
}, 10)
Expand Down Expand Up @@ -197,6 +216,7 @@ test('server-side abort below error threshold and socket closed - call end', fun
}

var server = http.createServer(function (req, res) {
res.writeHead(200) // See race comment above.
setTimeout(function () {
t.ok(timedout, 'should have closed socket')
t.notOk(ended, 'should not have ended transaction')
Expand Down Expand Up @@ -240,6 +260,7 @@ test('server-side abort above error threshold and socket closed - call end', fun
}

var server = http.createServer(function (req, res) {
res.writeHead(200) // See race comment above.
setTimeout(function () {
t.ok(timedout, 'should have closed socket')
t.notOk(ended, 'should not have ended transaction')
Expand Down
4 changes: 4 additions & 0 deletions test/instrumentation/modules/http2.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ isSecure.forEach(secure => {
test(`http2.${method} compatibility mode`, t => {
t.plan(15)

// Note NODE_OPTIONS env because it sometimes has a setting relevant
// for this test.
t.comment(`NODE_OPTIONS=${process.env.NODE_OPTIONS}`)

resetAgent((data) => {
assert(t, data, secure, port)
server.close()
Expand Down
19 changes: 19 additions & 0 deletions test/openssl-config-for-testing.cnf
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# An OpenSSL config to provide to node for test runs.
#
# This sets the OpenSSL security level (SECLEVEL) to 0. The
# "http2.createSecureServer()" tests in test/instrumentation/modules/http2.js
# fail if SECLEVEL=2, which it is in some environments, e.g. the "node:16"
# docker image.
#
# Based on https://github.com/nodejs/node/issues/36655

openssl_conf = openssl_init

[openssl_init]
ssl_conf = ssl_sect

[ssl_sect]
system_default = system_default_sect

[system_default_sect]
CipherString = DEFAULT:@SECLEVEL=0
Loading