Skip to content

Conversation

delvedor
Copy link
Member

As titled.

Closes: #1516

Copy link
Member

@trentm trentm left a comment

Choose a reason for hiding this comment

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

Confirmed this fixes the crash issue in #1516 (with APM instrumentation v3.19.0 of the ES client making a request to non-ES).

testing notes

Running a mock ES that responds like this:

HTTP/1.1 200 OK
X-elastic-product: Elasticsearch
content-type: application/json
Date: Mon, 23 Aug 2021 19:13:08 GMT
Connection: keep-alive
Keep-Alive: timeout=5
Transfer-Encoding: chunked

{
  "hi": "there"
}

I first confirmed this hits #1516 with v3.19.0 of the APM agent (before the fix) and v7.14.0 of the ES client. "esquery.js" is a small script that uses APM and calls esClient.search(...):

% npm ls @elastic/elasticsearch
elastic-apm-node@3.19.0 /Users/trentm/el/apm-agent-nodejs8
└── @elastic/elasticsearch@7.14.0

% node esquery.js
/Users/trentm/el/apm-agent-nodejs8/lib/instrumentation/modules/@elastic/elasticsearch.js:47
          { id: request.id, method: request.params.method, path: request.params.path })
                                                   ^

TypeError: Cannot read property 'method' of null
    at EventEmitter.<anonymous> (/Users/trentm/el/apm-agent-nodejs8/lib/instrumentation/modules/@elastic/elasticsearch.js:47:52)
    at EventEmitter.emit (events.js:314:20)
    at EventEmitter.<anonymous> (/Users/trentm/el/apm-agent-nodejs8/node_modules/@elastic/elasticsearch/lib/Transport.js:464:18)

Then I installed this PR's fix and ran again:

% npm install github:elastic/elasticsearch-js#fix-1516

% node esquery.js
{"log.level":"info","@timestamp":"2021-08-23T19:16:32.488Z","log":{"logger":"elastic-apm-node"},"ecs":{"version":"1.6.0"},"message":"Sending error to Elastic APM: {\"id\":\"71a2818c6c0a676015124ef32d8d7bc5\"}"}

This results in a sufficient APM trace:

  • the "Elasticsearch: GET /" span
  • an APM error event:
{
    "error": {
        "id": "58231373ba7e0f1846342238b5bd9158",
        "timestamp": 1629746450856000,
        "parent_id": "c3c625cbf083c78b",
        "trace_id": "d11ca3e94e7ac7778b117129eff1f152",
        "transaction_id": "185a6154d541781a",
        "exception": {
            "message": "The client noticed that the server is not Elasticsearch and we do not support this unknown product.",
            "type": "ProductNotSupportedError",
            "handled": true,
            "attributes": {
                "name": "ProductNotSupportedError"
            },
            "module": "@elastic/elasticsearch",
...

Note that I only say "sufficient", because I think the APM instrumentation could possibly be improved here. A span is not created for the attempted "Elasticsearch: POST /_search" request. The only span is for the "Elasticsearch: GET /" product check request... and that request succeeded, so the outcome of the span and transaction is "success". I think ultimately we want the outcome to be "failure" for at least a span. This is independent of this PR, however. I'll open a separate ticket on the APM agent for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot read property 'method' of null since 7.14.0
2 participants