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

Improve observability #834

Merged
merged 28 commits into from
May 3, 2019
Merged

Improve observability #834

merged 28 commits into from
May 3, 2019

Conversation

delvedor
Copy link
Member

@delvedor delvedor commented Apr 29, 2019

With this pr, we add support for the request id. With it, it will be easier correlate clients events and in the future enhance the client observability.

It can be fully customized in the configuration options (as well as in the child clients) and can be overridden with the specific request options.

@watson @Qard: can this be useful for you? What can I do to improve the client observability for Elastic APM?

Example:

const { Client } = require('@elastic/elasticsearch')
const client = new Client({
  node: 'http://localhost:9200'
  // generateRequestId: function () { ... }
})

client.on('request', (err, { meta }) => {
  console.log(meta.request.id)
})

client.on('response', (err, { meta }) => {
  console.log(meta.request.id)
})

client.info(console.log)
client.info({}, { id: 'custom' }, console.log)

/cc @tylersmalley @epixa @spalger

@Qard
Copy link

Qard commented Apr 29, 2019

It seems to be in pretty good shape from APM perspective. With the request and response events we can capture the start and end of a request easily, and we have the metadata to capture the path, body, host, etc.

Thanks for the notice. :)

@delvedor
Copy link
Member Author

delvedor commented Apr 30, 2019

I'm thinking if it is worth to add a context object instead, which will contain the request id and any more data you might be interested in.

Eg:

const { Client } = require('@elastic/elasticsearch')
const client = new Client({
  node: 'http://localhost:9200'
})

client.on('request', (err, { meta }) => {
  console.log(meta.request.context) // { id: 0, winter: 'is coming' }
  meta.request.context.star = 'wars'
})

client.on('response', (err, { meta }) => {
  console.log(meta.request.context) // { id: 0, winter: 'is coming', star: 'wars' }
})

client.info({}, { context: { winter: 'is coming' } }, console.log)

@delvedor
Copy link
Member Author

Yes, if you don't do anything, context will be { id: 'some-id' }, and the id will be generated by generateRequestId (unless you override it).

@delvedor delvedor changed the title Added correlation id support Improve observability Apr 30, 2019
@delvedor
Copy link
Member Author

delvedor commented Apr 30, 2019

I've done some more changes which should improve the observability:

  • event client instance now has a name (which can be configured), this is very useful when working with child clients.
  • In every request, you can provide a custom context object that you can access later.
  • Now you can access request id and client name also in the resurrect event.
  • observability doc

In code:

const { Client } = require('@elastic/elasticsearch')
const client = new Client({
  node: 'http://localhost:9200',
  name: 'cluster-abc'
})

client.on('request', (err, { meta }) => {
  const { name, context, request } = meta
  console.log(name, context, request.id)
})

client.info({}, { context: { winter: 'is coming' }, id: 'custom-id' }, console.log)

@tylersmalley
Copy link
Contributor

👍 this is great.

@delvedor delvedor marked this pull request as ready for review May 2, 2019 13:47
Copy link
Contributor

@tylersmalley tylersmalley left a comment

Choose a reason for hiding this comment

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

LGTM - just minor nitpicks to the docs. This gives me a lot of ideas for Kibana!

docs/observability.asciidoc Outdated Show resolved Hide resolved
docs/observability.asciidoc Outdated Show resolved Hide resolved
docs/observability.asciidoc Show resolved Hide resolved
lib/Transport.js Show resolved Hide resolved
tylersmalley and others added 4 commits May 2, 2019 17:44
Co-Authored-By: delvedor <delvedor@users.noreply.github.com>
Co-Authored-By: delvedor <delvedor@users.noreply.github.com>
@Conky5
Copy link
Contributor

Conky5 commented May 2, 2019

Jenkins retest this please

docs/configuration.asciidoc Outdated Show resolved Hide resolved
docs/configuration.asciidoc Outdated Show resolved Hide resolved
docs/configuration.asciidoc Outdated Show resolved Hide resolved
docs/observability.asciidoc Outdated Show resolved Hide resolved
docs/observability.asciidoc Outdated Show resolved Hide resolved
docs/observability.asciidoc Outdated Show resolved Hide resolved
docs/observability.asciidoc Outdated Show resolved Hide resolved
docs/observability.asciidoc Outdated Show resolved Hide resolved
docs/observability.asciidoc Show resolved Hide resolved
docs/usage.asciidoc Show resolved Hide resolved
Spencer and others added 9 commits May 3, 2019 11:57
Co-Authored-By: delvedor <delvedor@users.noreply.github.com>
Co-Authored-By: delvedor <delvedor@users.noreply.github.com>
Co-Authored-By: delvedor <delvedor@users.noreply.github.com>
Co-Authored-By: delvedor <delvedor@users.noreply.github.com>
Co-Authored-By: delvedor <delvedor@users.noreply.github.com>
README.md Show resolved Hide resolved
Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

A couple lingering questions, but LGTM

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@delvedor
Copy link
Member Author

delvedor commented May 3, 2019

Jenkins retest this please

@delvedor delvedor merged commit 269c0fc into master May 3, 2019
@delvedor delvedor deleted the correlation-id branch May 3, 2019 15:23
delvedor added a commit that referenced this pull request May 3, 2019
* API generation

* Added correlation id support

* Updated docs

* Updated test

* Updated code generation

* API generation

* Updated code generation

* Added support for client name and custom context object

* Updated docs

* Updated test

* Fix docs

* Updated docs

* Added id support also for sniffing

* Updated test

* Update docs/observability.asciidoc

Co-Authored-By: delvedor <delvedor@users.noreply.github.com>

* Update docs/observability.asciidoc

Co-Authored-By: delvedor <delvedor@users.noreply.github.com>

* Apply suggestions

* Update docs/configuration.asciidoc

Co-Authored-By: delvedor <delvedor@users.noreply.github.com>

* Update docs/configuration.asciidoc

Co-Authored-By: delvedor <delvedor@users.noreply.github.com>

* Update docs/observability.asciidoc

Co-Authored-By: delvedor <delvedor@users.noreply.github.com>

* Update docs/observability.asciidoc

Co-Authored-By: delvedor <delvedor@users.noreply.github.com>

* Update docs/observability.asciidoc

Co-Authored-By: delvedor <delvedor@users.noreply.github.com>

* Apply suggestions

* Updated README.md

* Fixed test

* Addressed suggestions
delvedor added a commit that referenced this pull request May 3, 2019
* API generation

* Added correlation id support

* Updated docs

* Updated test

* Updated code generation

* API generation

* Updated code generation

* Added support for client name and custom context object

* Updated docs

* Updated test

* Fix docs

* Updated docs

* Added id support also for sniffing

* Updated test

* Update docs/observability.asciidoc

Co-Authored-By: delvedor <delvedor@users.noreply.github.com>

* Update docs/observability.asciidoc

Co-Authored-By: delvedor <delvedor@users.noreply.github.com>

* Apply suggestions

* Update docs/configuration.asciidoc

Co-Authored-By: delvedor <delvedor@users.noreply.github.com>

* Update docs/configuration.asciidoc

Co-Authored-By: delvedor <delvedor@users.noreply.github.com>

* Update docs/observability.asciidoc

Co-Authored-By: delvedor <delvedor@users.noreply.github.com>

* Update docs/observability.asciidoc

Co-Authored-By: delvedor <delvedor@users.noreply.github.com>

* Update docs/observability.asciidoc

Co-Authored-By: delvedor <delvedor@users.noreply.github.com>

* Apply suggestions

* Updated README.md

* Fixed test

* Addressed suggestions
delvedor added a commit that referenced this pull request May 3, 2019
* API generation

* Added correlation id support

* Updated docs

* Updated test

* Updated code generation

* API generation

* Updated code generation

* Added support for client name and custom context object

* Updated docs

* Updated test

* Fix docs

* Updated docs

* Added id support also for sniffing

* Updated test

* Update docs/observability.asciidoc

Co-Authored-By: delvedor <delvedor@users.noreply.github.com>

* Update docs/observability.asciidoc

Co-Authored-By: delvedor <delvedor@users.noreply.github.com>

* Apply suggestions

* Update docs/configuration.asciidoc

Co-Authored-By: delvedor <delvedor@users.noreply.github.com>

* Update docs/configuration.asciidoc

Co-Authored-By: delvedor <delvedor@users.noreply.github.com>

* Update docs/observability.asciidoc

Co-Authored-By: delvedor <delvedor@users.noreply.github.com>

* Update docs/observability.asciidoc

Co-Authored-By: delvedor <delvedor@users.noreply.github.com>

* Update docs/observability.asciidoc

Co-Authored-By: delvedor <delvedor@users.noreply.github.com>

* Apply suggestions

* Updated README.md

* Fixed test

* Addressed suggestions
@delvedor delvedor mentioned this pull request Aug 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants