-
Notifications
You must be signed in to change notification settings - Fork 238
Added instrumentation for Elasticsearch Node.js client #1266
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
Changes from all commits
1ef3a14
d626929
cb19e81
fc65a79
c4a81ae
fff7312
f003127
81c2282
abf8e37
9053612
edf32d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,29 +1,30 @@ | ||
| TAV: | ||
| - @elastic/elasticsearch | ||
| - apollo-server-express | ||
| - bluebird | ||
| - cassandra-driver | ||
| - elasticsearch | ||
| - express | ||
| - express-graphql | ||
| - express-queue | ||
| - fastify | ||
| - finalhandler | ||
| - generic-pool | ||
| - mysql | ||
| - mysql2 | ||
| - redis | ||
| - koa-router | ||
| - got | ||
| - graphql | ||
| - handlebars | ||
| - hapi | ||
| - ioredis | ||
| - jade | ||
| - pug | ||
| - knex | ||
| - koa-router | ||
| - mimic-response | ||
| - mongodb-core | ||
| - finalhandler | ||
| - ioredis | ||
| - mysql | ||
| - mysql2 | ||
| - pg | ||
| - cassandra-driver | ||
| - tedious | ||
| - pug | ||
| - redis | ||
| - restify | ||
| - fastify | ||
| - mimic-response | ||
| - got | ||
| - bluebird | ||
| - apollo-server-express | ||
| - knex | ||
| - tedious | ||
| - ws | ||
| - graphql | ||
| - express-graphql | ||
| - elasticsearch | ||
| - hapi | ||
| - express | ||
| - express-queue | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| 'use strict' | ||
|
|
||
| const queryRegexp = /_((search|msearch)(\/template)?|count)$/ | ||
|
|
||
| exports.setDbContext = function (span, params) { | ||
| if (queryRegexp.test(params.path)) { | ||
| // The old client exposes body and query, the new client exposes body and querystring | ||
| const statement = Array.isArray(params.body) | ||
| ? params.body.map(JSON.stringify).join('\n') | ||
| : stringifyQuery(params).trim() | ||
|
|
||
| if (statement) { | ||
| span.setDbContext({ | ||
| type: 'elasticsearch', | ||
| statement | ||
| }) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| function stringifyQuery (params) { | ||
| const query = params.body || params.querystring || params.query | ||
| return typeof query === 'string' ? query : JSON.stringify(query) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| 'use strict' | ||
|
|
||
| const { setDbContext } = require('../../elasticsearch-shared') | ||
|
|
||
| module.exports = function (elasticsearch, agent, { version, enabled }) { | ||
| if (!enabled) return elasticsearch | ||
|
|
||
| function generateSpan (activeSpans, meta, params) { | ||
| const span = agent.startSpan(null, 'db.elasticsearch.request') | ||
| if (span === null) return null | ||
|
|
||
| const { request } = meta | ||
| span.name = `Elasticsearch: ${params.method} ${params.path}` | ||
|
|
||
| activeSpans.set(request.id, span) | ||
|
|
||
| return span | ||
| } | ||
|
|
||
| class ApmClient extends elasticsearch.Client { | ||
| constructor (opts) { | ||
| super(opts) | ||
|
|
||
| const activeSpans = new Map() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using a map here might be a bit risky. If something interrupts the flow of the client and it fails to emit the next event in the sequence after the span has been added to the map it would result in a memory leak. Might want to consider using mapcap here, just to be safe. |
||
| let hasPrepareRequestEvent = false | ||
|
|
||
| this.on('prepare-request', (err, { meta, params }) => { | ||
| hasPrepareRequestEvent = true | ||
| if (err) { | ||
| return agent.captureError(err) | ||
| } | ||
|
|
||
| generateSpan(activeSpans, meta, params) | ||
| }) | ||
|
|
||
| this.on('request', (err, { meta }) => { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd love the make the query more readable in the case of an array of queries. What I do with the old client is format it as NDJSON so that each query is on its own line. If we use the JSON that you formatted, it will be one long line containing all the queries... not really sure how to best do this 🤔
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What you mean with "array of queries"?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Example: const body = [
{},
{
query: {
query_string: {
query: 'pants'
}
}
}
]
client.msearch({ body }, function () {...})
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that case you will get a |
||
| const { request } = meta | ||
|
|
||
| if (err) { | ||
| const span = activeSpans.get(request.id) | ||
| agent.captureError(err) | ||
| if (span !== undefined) { | ||
| span.end() | ||
| activeSpans.delete(request.id) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| const span = hasPrepareRequestEvent === false | ||
| ? generateSpan(activeSpans, meta, request.params) | ||
| : activeSpans.get(request.id) | ||
|
|
||
| if (span) setDbContext(span, request.params) | ||
|
|
||
| // TODO: can we signal somehow that here | ||
| // we are starting the actual http request? | ||
| }) | ||
|
|
||
| this.on('response', (err, { meta }) => { | ||
| if (err) { | ||
| // TODO: rebuild error object to avoid serialization issues | ||
| agent.captureError(err, { custom: err.message }) | ||
| // agent.captureError(err, { labels: { meta: JSON.stringify(err.meta) } }) | ||
| // agent.captureError(err, { custom: JSON.parse(JSON.stringify(err.meta)) }) | ||
| // agent.captureError(err, { custom: err.meta.meta.connection }) | ||
| } | ||
|
|
||
| const { request } = meta | ||
| const span = activeSpans.get(request.id) | ||
| if (span !== undefined) { | ||
| span.end() | ||
| activeSpans.delete(request.id) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| return Object.assign(elasticsearch, { Client: ApmClient }) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -109,6 +109,7 @@ | |
| "@commitlint/cli": "^8.0.0", | ||
| "@commitlint/config-conventional": "^8.0.0", | ||
| "@commitlint/travis-cli": "^8.0.0", | ||
| "@elastic/elasticsearch": "elastic/elasticsearch-js#update-events", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is merged into master, it will break ones the branch is deleted. But for now it's the only way to test our compatibility with the yet unreleased Maybe we should wait to merge this PR until that PR branch has been merged? |
||
| "@types/node": "^12.0.8", | ||
| "apollo-server-express": "^2.6.3", | ||
| "aws-sdk": "^2.477.0", | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the diff... I only added this line, but sorted all entries alphabetically