Added instrumentation for Elasticsearch Node.js client #1266
Conversation
|
@delvedor we use conventional commits. Could I get you to rename the commit to something along the lines of |
|
Nevermind... I got the opportunity to do it my self when I had to check it out locally to add tests... I force pushed, so you'll need to pull it down again |
| generateSpan(activeSpans, meta, params) | ||
| }) | ||
|
|
||
| this.on('request', (err, { meta }) => { |
There was a problem hiding this comment.
In the request event the body and querystring values will be already serialized, would it be possible to add the setDbContext call here?
In this way, we don't need to serialize the body/querystring twice ;)
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
What you mean with "array of queries"?
There was a problem hiding this comment.
Example:
const body = [
{},
{
query: {
query_string: {
query: 'pants'
}
}
}
]
client.msearch({ body }, function () {...})There was a problem hiding this comment.
In that case you will get a ndjson serialized body.
| @@ -1,29 +1,30 @@ | |||
| TAV: | |||
| - @elastic/elasticsearch | |||
There was a problem hiding this comment.
Sorry for the diff... I only added this line, but sorted all entries alphabetically
This is so that we can test that the prepare-request event works
| }) | ||
|
|
||
| test('client.search with callback', function userLandCode (t) { | ||
| resetAgent(done(t, 'GET', '/_search', 'q=pants')) |
There was a problem hiding this comment.
In the old client we actually report this as {"q":"pants"} because that's what the user provides. The fact that the client chooses the add this to the querystring is not really relevant. It would be nicest if we reported the query as it was provided by the user.
The downside of that is that we'll have to serialize it twice, so the question is if it's worth it.
| @@ -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", | |||
There was a problem hiding this comment.
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 prepare-request event which will ship in 7.4.
Maybe we should wait to merge this PR until that PR branch has been merged?
| constructor (opts) { | ||
| super(opts) | ||
|
|
||
| const activeSpans = new Map() |
There was a problem hiding this comment.
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.
|
jenkins run the tests please |
This commit is very incomplete, based on earlier work in: #1266
|
Closing in favour of #1877 |
This pr adds the support for the new Elasticsearch client for Node.js.
TODO:
Closes: #968
Related: elastic/elasticsearch-js#932