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

feat: add support for APM Agent Configuration via Kibana#66

Merged
Qard merged 10 commits intoelastic:masterfrom
watson:remote-config
Jul 29, 2019
Merged

feat: add support for APM Agent Configuration via Kibana#66
Qard merged 10 commits intoelastic:masterfrom
watson:remote-config

Conversation

@watson
Copy link
Copy Markdown
Contributor

@watson watson commented Jul 5, 2019

HTTP client part of elastic/apm-agent-nodejs#1125.

@watson watson self-assigned this Jul 5, 2019
@watson
Copy link
Copy Markdown
Contributor Author

watson commented Jul 5, 2019

Wrote a small test program to test this based on the dev setup described in elastic/apm#76 (comment):

const Client = require('elastic-apm-http-client')

const client = new Client({
  serviceName: 'opbeans-fortran',
  agentName: 'my-nodejs-agent',
  agentVersion: '1.0.0',
  userAgent: 'My Custom Elastic APM Agent',
  remoteConfig: true
})

client.on('config', function (conf) {
  console.log('new config:', conf)
})

client.on('request-error', function (err) {
  console.log('request-error:', err)
})

setTimeout(function () {}, 100000) // don't exit the process

This was the console output:

-- polling
-- request: {
  agent: undefined,
  rejectUnauthorized: true,
  hostname: 'localhost',
  port: '8200',
  method: 'GET',
  path: '/config/v1/agents?service.name=opbeans-fortran&environment=development',
  headers: {
    Accept: 'application/json',
    'User-Agent': 'My Custom Elastic APM Agent elastic-apm-http-client/8.0.0'
  }
}
-- got poll response (status: 200): {
  'cache-control': 'max-age=30, must-revalidate',
  'content-type': 'application/json',
  etag: '5',
  date: 'Fri, 05 Jul 2019 10:20:36 GMT',
  'content-length': '30',
  connection: 'close'
}
-- sechedule poll in 30 seconds
new config: { sampling_rate: '0.12' }
-- polling
-- request: {
  agent: undefined,
  rejectUnauthorized: true,
  hostname: 'localhost',
  port: '8200',
  method: 'GET',
  path: '/config/v1/agents?service.name=opbeans-fortran&environment=development',
  headers: {
    Accept: 'application/json',
    'User-Agent': 'My Custom Elastic APM Agent elastic-apm-http-client/8.0.0',
    'If-None-Match': '5'
  }
}
-- got poll response (status: 304): {
  'cache-control': 'max-age=30, must-revalidate',
  etag: '5',
  date: 'Fri, 05 Jul 2019 10:21:06 GMT',
  connection: 'close'
}
-- sechedule poll in 30 seconds

@watson watson force-pushed the remote-config branch 4 times, most recently from df2a5a2 to 42a4fdf Compare July 7, 2019 10:49
@watson
Copy link
Copy Markdown
Contributor Author

watson commented Jul 8, 2019

@Qard just added two commits, the primary one to just add a notice about required APM Server version (the other was just code cleanup I snug in since I had to push the doc change)

Update: Had to revert that last commit, as default function args doesn't work exactly how falsy values does. In our case, there's a risk at the number of seconds given is NaN in case the max-age cannot be parsed. And we want to fall back to 300 seconds in that case.

@watson
Copy link
Copy Markdown
Contributor Author

watson commented Jul 8, 2019

... aaand another one. Just found a "typo" in the new docs that I wanted to fix.

@watson
Copy link
Copy Markdown
Contributor Author

watson commented Jul 8, 2019

I just updated the PR with the latest updates from elastic/apm#76:

  • The APM Server no longer responds with 500 on the config endpoint. Instead, it responds with:
    • 403 if the APM Server isn't configured to expose /config/v1/agents
    • 503 if the APM Server can't reach Kibana
  • It seems like we're settling on centralConfig instead of remoteConfig. I've updated the PR in anticipation of this (hopefully not prematurely)

It also seems like we're going to decide to have this feature enabled by default. I still think we should leave it off by default in the http client, no matter if we end up turning it on by default in the agent. This will allow us to release this PR to npm as a minor bump in good time before 7.3 without worrying about anything.

@watson watson requested a review from Qard July 8, 2019 20:54
Copy link
Copy Markdown
Contributor

@Qard Qard left a comment

Choose a reason for hiding this comment

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

Just did a fresh review and this still seems correct, as far as I can tell. The config option appears to be not happening though, so unsure if we should unify by removing it or just leave the un-unified config in there as we generally just focus on config unification at the agent level. Personally, I like having this escape hatch here in case we need it later. 🤔

@felixbarny @axw Thoughts?

@Qard Qard merged commit 58f4e0f into elastic:master Jul 29, 2019
@watson watson deleted the remote-config branch August 1, 2019 15:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants