Skip to content
This repository has been archived by the owner on Aug 4, 2023. It is now read-only.

Commit

Permalink
feat!: metadata.system.{configured_hostname,detected_hostname}, prefe…
Browse files Browse the repository at this point in the history
…r FQDN (#200)

This also adds testing of node v20 and testing on windows (just one node version).

Refs: elastic/apm-agent-nodejs#3310
  • Loading branch information
trentm authored Jun 27, 2023
1 parent b30b2f3 commit a90fe47
Show file tree
Hide file tree
Showing 12 changed files with 247 additions and 60 deletions.
20 changes: 17 additions & 3 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,27 @@ on:
jobs:
test-vers:
strategy:
fail-fast: false
matrix:
node: ['8.6', '8', '10', '12', '14', '15', '16', '18', '19']
node: ['8.6', '8', '10', '12', '14', '15', '16', '18', '19', '20']
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- uses: actions/setup-node@v2
- uses: actions/checkout@v3
- uses: actions/setup-node@v3
with:
node-version: ${{ matrix.node }}
- run: npm install
- run: npm test

test-windows:
runs-on: windows-latest
steps:
- uses: actions/checkout@v3
- uses: actions/setup-node@v3
with:
# What Node.js version to test on Windows is a balance between which
# is the current LTS version (https://github.com/nodejs/release) and
# which version more of our users are using.
node-version: 16
- run: npm install
- run: npm test
12 changes: 12 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,17 @@
# elastic-apm-http-client changelog

## v12.0.0

- **Breaking change.** The `hostname` configuration option has been renamed to
`configuredHostname`. As well, the hostname detection has changed to prefer
using a FQDN, if available. See [the spec](https://github.com/elastic/apm/blob/main/specs/agents/metadata.md#hostname).
(https://github.com/elastic/apm-agent-nodejs/issues/3310)

- The APM client will send `metadata.system.detected_hostname` and
`metadata.system.configured_hostname` as appropriate for APM server versions
>=7.4, rather than the now deprecated `metadata.system.hostname`.
See [the spec](https://github.com/elastic/apm/blob/main/specs/agents/metadata.md#hostname).

## v11.4.0

- Add support for pre-registering of partial transactions for AWS Lambda.
Expand Down
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ See also the "Cloud & Extra Metadata" section below.
specific framework, use this config option to log its name
- `frameworkVersion` - If the service being instrumented is running a
specific framework, use this config option to log its version
- `hostname` - Custom hostname (default: OS hostname)
- `configuredHostname` - A user-configured hostname, if any, e.g. from the `ELASTIC_APM_HOSTNAME` envvar.
See <https://github.com/elastic/apm/blob/main/specs/agents/metadata.md#hostname>.
- `environment` - Environment name (default: `process.env.NODE_ENV || 'development'`)
- `containerId` - Docker container id, if not given will be parsed from `/proc/self/cgroup`
- `kubernetesNodeName` - Kubernetes node name
Expand Down
36 changes: 30 additions & 6 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ const fs = require('fs')
const http = require('http')
const https = require('https')
const util = require('util')
const os = require('os')
const { performance } = require('perf_hooks')
const { URL } = require('url')
const zlib = require('zlib')
Expand All @@ -22,6 +21,7 @@ const semver = require('semver')
const streamToBuffer = require('fast-stream-to-buffer')
const StreamChopper = require('stream-chopper')

const { detectHostname } = require('./lib/detect-hostname')
const ndjson = require('./lib/ndjson')
const { NoopLogger } = require('./lib/logging')
const truncate = require('./lib/truncate')
Expand All @@ -37,7 +37,6 @@ function isFlushMarker (obj) {
return obj === kFlush || obj === kLambdaEndFlush
}

const hostname = os.hostname()
const requiredOpts = [
'agentName',
'agentVersion',
Expand Down Expand Up @@ -243,7 +242,6 @@ Client.prototype.config = function (opts) {
if (!this._conf.time && this._conf.time !== 0) this._conf.time = 10000
if (!this._conf.serverTimeout && this._conf.serverTimeout !== 0) this._conf.serverTimeout = 15000
if (!this._conf.serverUrl) this._conf.serverUrl = 'http://127.0.0.1:8200'
if (!this._conf.hostname) this._conf.hostname = hostname
if (!this._conf.environment) this._conf.environment = process.env.NODE_ENV || 'development'
if (!this._conf.truncateKeywordsAt) this._conf.truncateKeywordsAt = 1024
if (!this._conf.truncateStringsAt) this._conf.truncateStringsAt = 1024
Expand All @@ -265,6 +263,8 @@ Client.prototype.config = function (opts) {
// processed values
this._conf.serverUrl = new URL(this._conf.serverUrl)

this._conf.detectedHostname = detectHostname()

if (containerInfo) {
if (!this._conf.containerId && containerInfo.containerId) {
this._conf.containerId = containerInfo.containerId
Expand All @@ -273,7 +273,11 @@ Client.prototype.config = function (opts) {
this._conf.kubernetesPodUID = containerInfo.podId
}
if (!this._conf.kubernetesPodName && containerInfo.podId) {
this._conf.kubernetesPodName = hostname
// https://kubernetes.io/docs/concepts/workloads/pods/#working-with-pods
// suggests a pod name should just be the shorter "DNS label", and my
// guess is k8s defaults a pod name to just the *short* hostname, not
// the FQDN.
this._conf.kubernetesPodName = this._conf.detectedHostname.split('.', 1)[0]
}
}

Expand Down Expand Up @@ -1223,7 +1227,8 @@ function getChoppedStreamHandler (client, onerror) {
* Some behaviors in the APM depend on the APM Server version. These are
* exposed as `Client#supports...` boolean methods.
*
* These `Client#supports...` method names intentionally match those from the Java agent:
* These `Client#supports...` method names, if not always the implementation,
* intentionally match those from the Java agent:
* https://github.com/elastic/apm-agent-java/blob/master/apm-agent-core/src/main/java/co/elastic/apm/agent/report/ApmServerClient.java#L322-L349
*/
Client.prototype.supportsKeepingUnsampledTransaction = function () {
Expand All @@ -1245,6 +1250,13 @@ Client.prototype.supportsActivationMethodField = function () {
return semver.gte(this._apmServerVersion, '8.7.1')
}
}
Client.prototype.supportsConfiguredAndDetectedHostname = function () {
if (!this._apmServerVersion) {
return true // Optimistically assume APM server is >=7.4.
} else {
return semver.gte(this._apmServerVersion, '7.4.0')
}
}

/**
* Signal to the Elastic AWS Lambda extension that a lambda function execution
Expand Down Expand Up @@ -1485,7 +1497,6 @@ function metadataFromConf (opts, client) {
argv: process.argv
},
system: {
hostname: opts.hostname,
architecture: process.arch,
platform: process.platform,
container: undefined,
Expand All @@ -1494,6 +1505,19 @@ function metadataFromConf (opts, client) {
labels: opts.globalLabels
}

// On `system.*hostname` fields:
// - `hostname` was deprecated in APM server v7.4, replaced by the next two.
// - Around Elastic v8.9, ECS changed `host.name` to prefer the FQDN,
// hence APM agents now prefer FQDN for `detected_hostname`.
if (client.supportsConfiguredAndDetectedHostname()) {
payload.system.detected_hostname = opts.detectedHostname
if (opts.configuredHostname) {
payload.system.configured_hostname = opts.configuredHostname
}
} else {
payload.system.hostname = opts.configuredHostname || opts.detectedHostname
}

if (opts.agentActivationMethod && client.supportsActivationMethodField()) {
payload.service.agent.activation_method = opts.agentActivationMethod
}
Expand Down
77 changes: 77 additions & 0 deletions lib/detect-hostname.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
'use strict'

// Detect the current hostname, preferring the FQDN if possible.
// Spec: https://github.com/elastic/apm/blob/main/specs/agents/metadata.md#hostname

const os = require('os')
const { spawnSync } = require('child_process')

/**
* *Synchronously* detect the current hostname, preferring the FQDN.
* This is sent to APM server as `metadata.system.detected_hostname`
* and is intended to fit the ECS `host.name` value
* (https://www.elastic.co/guide/en/ecs/current/ecs-host.html#field-host-name).
*
* @returns {String}
*/
function detectHostname () {
let hostname = null
let out
const fallback = os.hostname()

switch (os.platform()) {
case 'win32':
// https://learn.microsoft.com/en-us/dotnet/api/system.net.dns.gethostentry
out = spawnSync(
'powershell.exe',
['[System.Net.Dns]::GetHostEntry($env:computerName).HostName'],
{ encoding: 'utf8', shell: true, timeout: 2000 }
)
if (!out.error) {
hostname = out.stdout.trim()
break
}

// https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/hostname
out = spawnSync(
'hostname.exe',
{ encoding: 'utf8', shell: true, timeout: 2000 }
)
if (!out.error) {
hostname = out.stdout.trim()
break
}

if ('COMPUTERNAME' in process.env) {
hostname = process.env['COMPUTERNAME'].trim() // eslint-disable-line dot-notation
}
break

default:
out = spawnSync('/bin/hostname', ['-f'], { encoding: 'utf8', shell: false, timeout: 500 })
if (!out.error) {
hostname = out.stdout.trim()
}
// I'm going a little off of the APM spec here by *not* falling back to
// HOSTNAME or HOST envvars. Latest discussion point is here:
// https://github.com/elastic/apm/pull/517#issuecomment-940973458
// My understanding is HOSTNAME is a *Bash*-set envvar.
break
}

if (!hostname) {
hostname = fallback
}
hostname = hostname.trim().toLowerCase()
return hostname
}

module.exports = {
detectHostname
}

// ---- main

if (require.main === module) {
console.log(detectHostname())
}
5 changes: 3 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "elastic-apm-http-client",
"version": "11.4.0",
"version": "12.0.0",
"description": "A low-level HTTP client for communicating with the Elastic APM intake API",
"main": "index.js",
"directories": {
Expand All @@ -11,7 +11,7 @@
],
"scripts": {
"lint": "standard",
"test": "nyc ./test/run_tests.sh"
"test": "nyc node ./scripts/run-tests.js"
},
"engines": {
"node": "^8.6.0 || 10 || >=12"
Expand All @@ -30,6 +30,7 @@
"stream-chopper": "^3.0.1"
},
"devDependencies": {
"glob": "^7.2.3",
"ndjson": "^1.5.0",
"nyc": "^14.1.1",
"standard": "^14.3.1",
Expand Down
76 changes: 76 additions & 0 deletions scripts/run-tests.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/*
* Copyright Elasticsearch B.V. and other contributors where applicable.
* Licensed under the BSD 2-Clause License; you may not use this file except in
* compliance with the BSD 2-Clause License.
*/

'use strict'

// Run all "test/**/*.test.js" files, each in a separate process.

var spawn = require('child_process').spawn

var glob = require('glob')

// ---- support functions

// Run a single test file.
function runTestFile (testFile, cb) {
console.log(`# running test: node ${testFile}`)
var ps = spawn('node', [testFile], { stdio: 'inherit' })
ps.on('error', cb)
ps.on('close', function (code) {
if (code !== 0) {
const err = new Error('non-zero error code')
err.code = 'ENONZERO'
err.exitCode = code
return cb(err)
}
cb()
})
}

function series (tasks, cb) {
var results = []
var pos = 0

function done (err, result) {
if (err) return cb(err)
results.push(result)

if (++pos === tasks.length) {
cb(null, results)
} else {
tasks[pos](done)
}
}

setImmediate(tasks[pos], done)
}

function handlerBind (handler) {
return function (task) {
return handler.bind(null, task)
}
}

function mapSeries (tasks, handler, cb) {
series(tasks.map(handlerBind(handler)), cb)
}

// ---- mainline

function main () {
var testFiles = glob.sync(
// Find all ".test.js" files, except those in "fixtures" dirs and in
// "node_modules" dirs created for test packages.
'test/**/*.test.js',
{ ignore: ['**/node_modules/**', '**/fixtures/**'] }
)

mapSeries(testFiles, runTestFile, function (err) {
if (err) throw err
})
}

main()
23 changes: 6 additions & 17 deletions test/central-config.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,8 @@ test('central config enabled', function (t) {

// Test central-config handling of Etag and If-None-Match headers using a mock
// apm-server that uses the `Cache-Control: max-age=1 ...` header to speed up
// the polling interval of the client.
// the polling interval of the client. (This is foiled by `INTERVAL_MIN_S = 5`.)
test('polling', function (t) {
t.plan((assertConfigReq.asserts + 1) * 8 + 12)

const expectedConf = { foo: 'bar' }
const headers = { 'Cache-Control': 'max-age=1, must-revalidate' }
let reqs = 0
Expand Down Expand Up @@ -130,17 +128,9 @@ test('polling', function (t) {
t.equal(req.headers['if-none-match'], '"42"')
res.writeHead(304, Object.assign({ Etag: '"42"' }, headers))
res.end()
break
case 8:
// Hard shutdown on request #8 to end the test.
// If the client's keep-alive agent has an open socket, this will
// result in a "socket hang up" observed on the client side.
res.writeHead(404)
res.end()
client.destroy()
server.close(function () {
t.end()
})
server.close()
t.end()
break
default:
t.fail('too many request')
Expand All @@ -164,11 +154,10 @@ test('polling', function (t) {
t.equal(err.code, 503)
t.equal(err.message, 'Unexpected APM Server response when polling config')
t.equal(err.response, 'from error property')
} else if (reqs === 8) {
// The mock APMServer above hard-destroys the connection on req 8. If
} else if (reqs === 7) {
// The mock APMServer above hard-destroys the connection on req 7. If
// the client's keep-alive agent has an open socket, we expect a
// "socket hang up" error here.
t.ok(err, 'got an err, as expected, on req 8')
// "socket hang up" (ECONNRESET) error here.
t.equal(err.message, 'socket hang up')
} else {
t.error(err, 'got an err on req ' + reqs + ', err=' + err.message)
Expand Down
Loading

0 comments on commit a90fe47

Please sign in to comment.