From 6e3a565f571be87a770ee59314ef5182121df033 Mon Sep 17 00:00:00 2001 From: delvedor Date: Tue, 6 Aug 2019 18:24:36 +0200 Subject: [PATCH 1/7] Added prepare-request event and emit a request error if the serializer fails --- index.d.ts | 1 + index.js | 1 + lib/Transport.js | 28 +++++++++++++++++++--------- 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/index.d.ts b/index.d.ts index ad404495c..6b4fe8381 100644 --- a/index.d.ts +++ b/index.d.ts @@ -629,6 +629,7 @@ declare class Client extends EventEmitter { declare const events: { RESPONSE: string; + PREPARE_REQUEST: string; REQUEST: string; SNIFF: string; RESURRECT: string; diff --git a/index.js b/index.js index 6fc3e7708..86172ca5d 100644 --- a/index.js +++ b/index.js @@ -244,6 +244,7 @@ function getAuth (node) { const events = { RESPONSE: 'response', + PREPARE_REQUEST: 'prepare-request', REQUEST: 'request', SNIFF: 'sniff', RESURRECT: 'resurrect' diff --git a/lib/Transport.js b/lib/Transport.js index 7e5b83681..16354ca99 100644 --- a/lib/Transport.js +++ b/lib/Transport.js @@ -104,13 +104,8 @@ class Transport { const compression = options.compression || this.compression var request = { abort: noop } - const makeRequest = () => { - if (meta.aborted === true) return - meta.connection = this.getConnection({ requestId: meta.request.id }) - if (meta.connection === null) { - return callback(new NoLivingConnectionsError('There are not living connections'), result) - } - + const prepareRequest = () => { + this.emit('prepare-request', null, { params, options, meta }) // TODO: make this assignment FAST const headers = Object.assign({}, this.headers, options.headers) @@ -120,6 +115,7 @@ class Transport { try { params.body = this.serializer.serialize(params.body) } catch (err) { + this.emit('request', err, result) return callback(err, result) } } @@ -143,6 +139,7 @@ class Transport { try { params.body = this.serializer.ndserialize(params.bulkBody) } catch (err) { + this.emit('request', err, result) return callback(err, result) } } else { @@ -170,11 +167,24 @@ class Transport { meta.request.params = params meta.request.options = options - this.emit('request', null, result) // handles request timeout params.timeout = toMs(options.requestTimeout || this.requestTimeout) if (options.asStream === true) params.asStream = true + return makeRequest() + } + + const makeRequest = () => { + if (meta.aborted === true) return + + meta.connection = this.getConnection({ requestId: meta.request.id }) + if (meta.connection === null) { + const err = new NoLivingConnectionsError('There are not living connections') + this.emit('request', err, result) + return callback(err, result) + } + + this.emit('request', null, result) // perform the actual http request return meta.connection.request(params, onResponse) } @@ -298,7 +308,7 @@ class Transport { }) } - request = makeRequest() + request = prepareRequest() return { abort: () => { From ae6b174265c5eb6c69d230d8a0bd9e683b0f1845 Mon Sep 17 00:00:00 2001 From: delvedor Date: Tue, 6 Aug 2019 18:24:41 +0200 Subject: [PATCH 2/7] Updated test --- test/unit/events.test.js | 43 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/test/unit/events.test.js b/test/unit/events.test.js index 9562e231d..b2769a9f6 100644 --- a/test/unit/events.test.js +++ b/test/unit/events.test.js @@ -9,6 +9,49 @@ const { Client, events } = require('../../index') const { TimeoutError } = require('../../lib/errors') const { connection: { MockConnection, MockConnectionTimeout } } = require('../utils') +test('Should emit a prepare-request event when starting a request', t => { + t.plan(3) + + const client = new Client({ + node: 'http://localhost:9200', + Connection: MockConnection + }) + + client.on(events.PREPARE_REQUEST, (err, request) => { + t.error(err) + t.match(request, { + meta: { + context: null, + name: 'elasticsearch-js', + request: { + params: null, + options: null, + id: 1 + }, + connection: null, + attempts: 0, + aborted: false + }, + params: { + path: '/test/_search', + querystring: { + q: 'foo:bar' + }, + method: 'GET', + body: '' + }, + options: {} + }) + }) + + client.search({ + index: 'test', + q: 'foo:bar' + }, (err, result) => { + t.error(err) + }) +}) + test('Should emit a request event when a request is performed', t => { t.plan(3) From b1b2f95903a9bb97b6d76d8c873e2e71654f8bf2 Mon Sep 17 00:00:00 2001 From: delvedor Date: Wed, 7 Aug 2019 11:31:55 +0200 Subject: [PATCH 3/7] Updated documentation --- docs/observability.asciidoc | 36 ++++++++++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) diff --git a/docs/observability.asciidoc b/docs/observability.asciidoc index c72c340e1..13ce7311f 100644 --- a/docs/observability.asciidoc +++ b/docs/observability.asciidoc @@ -37,6 +37,15 @@ client.on('response', (err, result) => { The client emits the following events: [cols=2*] |=== +|`prepare-request` +a|Emitted once the user invokes any API. +[source,js] +---- +client.on('prepare-request', (err, result) => { + console.log(err, result) +}) +---- + |`request` a|Emitted before sending the actual request to Elasticsearch _(emitted multiple times in case of retries)_. [source,js] @@ -86,8 +95,8 @@ meta: { context: any; name: string; request: { - params: TransportRequestParams; - options: TransportRequestOptions; + params: TransportRequestParams; // serialized by the client + options: TransportRequestOptions; // serialized by the client id: any; }; connection: Connection; @@ -100,6 +109,29 @@ meta: { }; ---- +`result` value in `prepare-request` will be: +[source,ts] +---- +params: TransportRequestParams; // as provided by the user +options: TransportRequestOptions; // as provided by the user +meta: { + context: any; + name: string; + request: { + params: null; + options: null; + id: any; + }; + connection: null; + attempts: number; + aborted: boolean; + sniff?: { + hosts: any[]; + reason: string; + }; +}; +---- + While the `result` value in `resurrect` will be: [source,ts] ---- From 7652cdaa09acb87240873cba8cc25b7428252d99 Mon Sep 17 00:00:00 2001 From: delvedor Date: Wed, 7 Aug 2019 11:32:06 +0200 Subject: [PATCH 4/7] Updated type definitions --- index.d.ts | 2 ++ lib/Transport.d.ts | 21 +++++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/index.d.ts b/index.d.ts index 6b4fe8381..8c817bfde 100644 --- a/index.d.ts +++ b/index.d.ts @@ -9,6 +9,7 @@ import { ConnectionOptions as TlsConnectionOptions } from 'tls'; import Transport, { ApiResponse, RequestEvent, + PrepareRequestEvent, TransportRequestParams, TransportRequestOptions, nodeFilterFn, @@ -645,6 +646,7 @@ export { errors, ApiResponse, RequestEvent, + PrepareRequestEvent, ResurrectEvent, RequestParams, ClientOptions, diff --git a/lib/Transport.d.ts b/lib/Transport.d.ts index 8099de2ea..3d23227d4 100644 --- a/lib/Transport.d.ts +++ b/lib/Transport.d.ts @@ -63,6 +63,27 @@ export interface RequestEvent { }; } +export interface PrepareRequestEvent { + params: TransportRequestParams; + options: TransportRequestOptions; + meta: { + context: C; + name: string; + request: { + params: null; + options: null; + id: any; + }; + connection: null; + attempts: number; + aborted: boolean; + sniff?: { + hosts: any[]; + reason: string; + }; + }; +} + // ApiResponse and RequestEvent are the same thing // we are doing this for have more clear names export interface ApiResponse extends RequestEvent {} From 4505d59a8cd0b19103cb718e6bbbebf9fbbf3b99 Mon Sep 17 00:00:00 2001 From: delvedor Date: Wed, 7 Aug 2019 11:32:12 +0200 Subject: [PATCH 5/7] Updated test --- test/behavior/observability.test.js | 47 ++++++++++++++++++++++++----- 1 file changed, 40 insertions(+), 7 deletions(-) diff --git a/test/behavior/observability.test.js b/test/behavior/observability.test.js index 65c16ca6d..6d992d365 100644 --- a/test/behavior/observability.test.js +++ b/test/behavior/observability.test.js @@ -24,7 +24,7 @@ test('Request id', t => { }) t.test('Custom generateRequestId', t => { - t.plan(7) + t.plan(9) const options = { context: { winter: 'is coming' } } @@ -38,6 +38,11 @@ test('Request id', t => { } }) + client.on('prepare-request', (err, { meta }) => { + t.error(err) + t.strictEqual(meta.request.id, 'custom-id') + }) + client.on('request', (err, { meta }) => { t.error(err) t.strictEqual(meta.request.id, 'custom-id') @@ -52,13 +57,18 @@ test('Request id', t => { }) t.test('Custom request id in method options', t => { - t.plan(5) + t.plan(7) const client = new Client({ node: 'http://localhost:9200', Connection: MockConnection }) + client.on('prepare-request', (err, { meta }) => { + t.error(err) + t.strictEqual(meta.request.id, 'custom-id') + }) + client.on('request', (err, { meta }) => { t.error(err) t.strictEqual(meta.request.id, 'custom-id') @@ -89,7 +99,7 @@ test('Request id', t => { }) t.test('sniffOnConnectionFault - should use the request id', t => { - t.plan(5) + t.plan(7) const client = new Client({ nodes: ['http://localhost:9200', 'http://localhost:9201'], @@ -98,6 +108,10 @@ test('Request id', t => { maxRetries: 0 }) + client.on('prepare-request', (e, { meta }) => { + t.strictEqual(meta.request.id, 'custom') + }) + client.on('request', (e, { meta }) => { t.strictEqual(meta.request.id, 'custom') }) @@ -151,13 +165,18 @@ test('Request id', t => { test('Request context', t => { t.test('no value', t => { - t.plan(5) + t.plan(7) const client = new Client({ node: 'http://localhost:9200', Connection: MockConnection }) + client.on('prepare-request', (err, { meta }) => { + t.error(err) + t.strictEqual(meta.context, null) + }) + client.on('request', (err, { meta }) => { t.error(err) t.strictEqual(meta.context, null) @@ -172,13 +191,18 @@ test('Request context', t => { }) t.test('custom value', t => { - t.plan(5) + t.plan(7) const client = new Client({ node: 'http://localhost:9200', Connection: MockConnection }) + client.on('prepare-request', (err, { meta }) => { + t.error(err) + t.deepEqual(meta.context, { winter: 'is coming' }) + }) + client.on('request', (err, { meta }) => { t.error(err) t.deepEqual(meta.context, { winter: 'is coming' }) @@ -206,13 +230,18 @@ test('Client name', t => { }) t.test('Is present in the event metadata', t => { - t.plan(6) + t.plan(8) const client = new Client({ node: 'http://localhost:9200', Connection: MockConnection, name: 'cluster' }) + client.on('prepare-request', (err, { meta }) => { + t.error(err) + t.strictEqual(meta.name, 'cluster') + }) + client.on('request', (err, { meta }) => { t.error(err) t.strictEqual(meta.name, 'cluster') @@ -246,7 +275,7 @@ test('Client name', t => { }) t.test('sniffOnConnectionFault', t => { - t.plan(5) + t.plan(7) const client = new Client({ nodes: ['http://localhost:9200', 'http://localhost:9201'], @@ -255,6 +284,10 @@ test('Client name', t => { maxRetries: 0 }) + client.on('prepare-request', (e, { meta }) => { + t.strictEqual(meta.name, 'elasticsearch-js') + }) + client.on('request', (e, { meta }) => { t.strictEqual(meta.name, 'elasticsearch-js') }) From a2dbee2bf0aa26361b20ec78af8460b43131eff6 Mon Sep 17 00:00:00 2001 From: delvedor Date: Mon, 23 Sep 2019 14:39:51 +0200 Subject: [PATCH 6/7] Emit event also in case of request aborted --- lib/Transport.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/Transport.js b/lib/Transport.js index 16354ca99..0f8a7e521 100644 --- a/lib/Transport.js +++ b/lib/Transport.js @@ -175,7 +175,10 @@ class Transport { } const makeRequest = () => { - if (meta.aborted === true) return + if (meta.aborted === true) { + this.emit('request', null, result) + return + } meta.connection = this.getConnection({ requestId: meta.request.id }) if (meta.connection === null) { From c7bcabf692addcdf44ebab597b8efc3e04618bc3 Mon Sep 17 00:00:00 2001 From: delvedor Date: Mon, 23 Sep 2019 14:39:57 +0200 Subject: [PATCH 7/7] Updated test --- test/unit/events.test.js | 362 +++++++++++++++++++++++------------- test/unit/transport.test.js | 2 +- 2 files changed, 233 insertions(+), 131 deletions(-) diff --git a/test/unit/events.test.js b/test/unit/events.test.js index b2769a9f6..b67bd7b8e 100644 --- a/test/unit/events.test.js +++ b/test/unit/events.test.js @@ -6,7 +6,7 @@ const { test } = require('tap') const { Client, events } = require('../../index') -const { TimeoutError } = require('../../lib/errors') +const { TimeoutError, SerializationError } = require('../../lib/errors') const { connection: { MockConnection, MockConnectionTimeout } } = require('../utils') test('Should emit a prepare-request event when starting a request', t => { @@ -52,157 +52,259 @@ test('Should emit a prepare-request event when starting a request', t => { }) }) -test('Should emit a request event when a request is performed', t => { - t.plan(3) +test('Request event', t => { + t.test('Should emit a request event when a request is performed', t => { + t.plan(3) - const client = new Client({ - node: 'http://localhost:9200', - Connection: MockConnection - }) + const client = new Client({ + node: 'http://localhost:9200', + Connection: MockConnection + }) - client.on(events.REQUEST, (err, request) => { - t.error(err) - t.match(request, { - body: null, - statusCode: null, - headers: null, - warnings: null, - meta: { - context: null, - name: 'elasticsearch-js', - request: { - params: { - method: 'GET', - path: '/test/_search', - body: '', - querystring: 'q=foo%3Abar', - headers: { - 'Content-Type': 'application/json', - 'Content-Length': '0' - } + client.on(events.REQUEST, (err, request) => { + t.error(err) + t.match(request, { + body: null, + statusCode: null, + headers: null, + warnings: null, + meta: { + context: null, + name: 'elasticsearch-js', + request: { + params: { + method: 'GET', + path: '/test/_search', + body: '', + querystring: 'q=foo%3Abar', + headers: { + 'Content-Type': 'application/json', + 'Content-Length': '0' + } + }, + options: {}, + id: 1 }, - options: {}, - id: 1 - }, - connection: { - id: 'http://localhost:9200' - }, - attempts: 0, - aborted: false - } + connection: { + id: 'http://localhost:9200' + }, + attempts: 0, + aborted: false + } + }) }) - }) - client.search({ - index: 'test', - q: 'foo:bar' - }, (err, result) => { - t.error(err) + client.search({ + index: 'test', + q: 'foo:bar' + }, (err, result) => { + t.error(err) + }) }) -}) -test('Should emit a response event in case of a successful response', t => { - t.plan(3) + t.test('Should emit a request event if there is a serialization error', t => { + t.plan(3) - const client = new Client({ - node: 'http://localhost:9200', - Connection: MockConnection - }) + const client = new Client({ + node: 'http://localhost:9200', + Connection: MockConnection + }) - client.on(events.RESPONSE, (err, request) => { - t.error(err) - t.match(request, { - body: { hello: 'world' }, - statusCode: 200, - headers: { - 'content-type': 'application/json;utf=8', - connection: 'keep-alive' - }, - warnings: null, - meta: { - context: null, - name: 'elasticsearch-js', - request: { - params: { - method: 'GET', - path: '/test/_search', - body: '', - querystring: 'q=foo%3Abar', - headers: { - 'Content-Type': 'application/json', - 'Content-Length': '0' - } + client.on(events.REQUEST, (err, request) => { + t.ok(err instanceof SerializationError) + t.match(request, { + body: null, + statusCode: null, + headers: null, + warnings: null, + meta: { + context: null, + name: 'elasticsearch-js', + request: { + params: null, + options: null, + id: 1 }, - options: {}, - id: 1 - }, - connection: { - id: 'http://localhost:9200' - }, - attempts: 0, - aborted: false - } + connection: null, + attempts: 0, + aborted: false + } + }) + }) + + const obj = {} + obj.o = obj + client.search({ + index: 'test', + body: obj + }, (err, result) => { + t.ok(err instanceof SerializationError) }) }) - client.search({ - index: 'test', - q: 'foo:bar' - }, (err, result) => { - t.error(err) + t.test('Should emit a request event if the request has been aborted', t => { + t.plan(6) + + var req + const client = new Client({ + node: 'http://localhost:9200', + Connection: MockConnectionTimeout + }) + + var count = 0 + client.on(events.REQUEST, (err, request) => { + if (count++ > 0) { + req.abort() + } + t.error(err) + t.match(request, { + body: null, + statusCode: null, + headers: null, + warnings: null, + meta: { + context: null, + name: 'elasticsearch-js', + request: { + params: { + method: 'GET', + path: '/test/_search', + body: '', + querystring: 'q=foo%3Abar', + headers: { + 'Content-Type': 'application/json', + 'Content-Length': '0' + } + }, + options: {}, + id: 1 + }, + connection: { + id: 'http://localhost:9200' + }, + attempts: count - 1, + aborted: count > 1 + } + }) + }) + + req = client.search({ + index: 'test', + q: 'foo:bar' + }, (e, result) => { + t.fail('Should not be called') + }) }) + + t.end() }) -test('Should emit a response event with the error set', t => { - t.plan(3) +test('Response event', t => { + t.test('Should emit a response event in case of a successful response', t => { + t.plan(3) - const client = new Client({ - node: 'http://localhost:9200', - Connection: MockConnectionTimeout, - maxRetries: 0 - }) + const client = new Client({ + node: 'http://localhost:9200', + Connection: MockConnection + }) - client.on(events.RESPONSE, (err, request) => { - t.ok(err instanceof TimeoutError) - t.match(request, { - body: null, - statusCode: null, - headers: null, - warnings: null, - meta: { - context: null, - name: 'elasticsearch-js', - request: { - params: { - method: 'GET', - path: '/test/_search', - body: '', - querystring: 'q=foo%3Abar', - headers: { - 'Content-Type': 'application/json', - 'Content-Length': '0' - } + client.on(events.RESPONSE, (err, request) => { + t.error(err) + t.match(request, { + body: { hello: 'world' }, + statusCode: 200, + headers: { + 'content-type': 'application/json;utf=8', + connection: 'keep-alive' + }, + warnings: null, + meta: { + context: null, + name: 'elasticsearch-js', + request: { + params: { + method: 'GET', + path: '/test/_search', + body: '', + querystring: 'q=foo%3Abar', + headers: { + 'Content-Type': 'application/json', + 'Content-Length': '0' + } + }, + options: {}, + id: 1 }, - options: { - requestTimeout: 500 + connection: { + id: 'http://localhost:9200' }, - id: 1 - }, - connection: { - id: 'http://localhost:9200' - }, - attempts: 0, - aborted: false - } + attempts: 0, + aborted: false + } + }) + }) + + client.search({ + index: 'test', + q: 'foo:bar' + }, (err, result) => { + t.error(err) }) }) - client.search({ - index: 'test', - q: 'foo:bar' - }, { - requestTimeout: 500 - }, (err, result) => { - t.ok(err instanceof TimeoutError) + t.test('Should emit a response event with the error set', t => { + t.plan(3) + + const client = new Client({ + node: 'http://localhost:9200', + Connection: MockConnectionTimeout, + maxRetries: 0 + }) + + client.on(events.RESPONSE, (err, request) => { + t.ok(err instanceof TimeoutError) + t.match(request, { + body: null, + statusCode: null, + headers: null, + warnings: null, + meta: { + context: null, + name: 'elasticsearch-js', + request: { + params: { + method: 'GET', + path: '/test/_search', + body: '', + querystring: 'q=foo%3Abar', + headers: { + 'Content-Type': 'application/json', + 'Content-Length': '0' + } + }, + options: { + requestTimeout: 500 + }, + id: 1 + }, + connection: { + id: 'http://localhost:9200' + }, + attempts: 0, + aborted: false + } + }) + }) + + client.search({ + index: 'test', + q: 'foo:bar' + }, { + requestTimeout: 500 + }, (err, result) => { + t.ok(err instanceof TimeoutError) + }) }) + + t.end() }) diff --git a/test/unit/transport.test.js b/test/unit/transport.test.js index 28f95ed45..4db981b3c 100644 --- a/test/unit/transport.test.js +++ b/test/unit/transport.test.js @@ -758,7 +758,7 @@ test('Should return a request aborter utility', t => { }) test('Retry mechanism and abort', t => { - t.plan(1) + t.plan(2) function handler (req, res) { setTimeout(() => {