newrelic / node-newrelic

New Relic Node.js agent code base. Developers are welcome to create pull requests here, please see our contributing guidelines. For New Relic technical support, please go to http://support.newrelic.com.
Apache License 2.0
971 stars 399 forks source link

Crash in elasticsearch instrumentation with null body. #1809

Closed villelahdenvuo closed 11 months ago

villelahdenvuo commented 1 year ago

Description

https://github.com/newrelic/node-newrelic/blob/main/lib/instrumentation/%40elastic/elasticsearch.js#L58

error.stack: "TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at Transport.queryParser (/mnt/ramdisk/ecom-api/node_modules/newrelic/lib/instrumentation/@elastic/elasticsearch.js:58:49)
    at DatastoreShim.parseQuery (/mnt/ramdisk/ecom-api/node_modules/newrelic/lib/shim/datastore-shim.js:456:35)
    at Transport.queryRecord (/mnt/ramdisk/ecom-api/node_modules/newrelic/lib/shim/datastore-shim.js:670:25)
    at Transport.wrapper (/mnt/ramdisk/ecom-api/node_modules/newrelic/lib/shim/shim.js:850:33)
    at IndicesApi.indicesExistsApi [as exists] (/mnt/ramdisk/ecom-api/node_modules/@elastic/elasticsearch/api/api/indices.js:361:25)

Expected Behavior

No error.

Steps to Reproduce

Calling indicesExistsApi in @elastic/elasticsearch

workato-integration[bot] commented 1 year ago

https://issues.newrelic.com/browse/NR-170194

mrickard commented 1 year ago

Thank you for the report, @villelahdenvuo , and sorry for the trouble. We'll prioritize a fix for this.

villelahdenvuo commented 1 year ago

@mrickard Luckily it got caught in our E2E tests. 😄

bizob2828 commented 1 year ago

@villelahdenvuo which version of @elastic/elasticsearch are you using?

villelahdenvuo commented 1 year ago

@bizob2828 @elastic/elasticsearch@7.10.0 as we're stuck on ES7 and would need to migrate to AWS OpenSearch.

bizob2828 commented 1 year ago

@villelahdenvuo ok, I think that might be the issue. We only support 8.0.0+, but unfortunately we didn't lock down the instrumentation but we built it with v8.0.0+. We will talk with our product manager about if we will support pre 8.0.0

villelahdenvuo commented 1 year ago

@bizob2828 Regardless I think it's a good practice to check for null when doing typeof x === 'object'. Don't you think?

bizob2828 commented 1 year ago

@villelahdenvuo for sure. we were just trying to reproduce with an integration test and could not based on your stack

villelahdenvuo commented 1 year ago

Btw I can upgrade to 7.17.13, I had misconfigured Dependabot to ignore minor updates.

Edit: Actually 7.13.x is the last supported OSS version.

mrickard commented 1 year ago

@villelahdenvuo Thank you! We're preparing to ship the release early next week.

mrickard commented 1 year ago

@villelahdenvuo This has been released with agent version 11.3.0. Thank you for the report!

bartosz347 commented 11 months ago

Hi @mrickard

I've tried to upgrade NewRelic from v11.1.0 to v11.5.0, but I'm still getting errors related to ElasticSearch.

We're using elasticsearch v7.13.0, here is the stack trace:

TypeError: Cannot read properties of null (reading 'then')
at Object.then (/app/node_modules/@elastic/elasticsearch/lib/Transport.js:156:18)
at DatastoreShim.interceptPromise (/app/node_modules/newrelic/lib/shim/shim.js:1713:8)
at DatastoreShim.bindPromise (/app/node_modules/newrelic/lib/shim/shim.js:1729:15)
at Transport._doRecord (/app/node_modules/newrelic/lib/shim/shim.js:937:22)
at Transport.wrapper (/app/node_modules/newrelic/lib/shim/shim.js:887:24)
at Client.bulkApi [as bulk] (/app/node_modules/@elastic/elasticsearch/api/api/bulk.js:67:25)
at tryBulk (/app/node_modules/@elastic/elasticsearch/lib/Helpers.js:691:16)
at bulkOperation (/app/node_modules/@elastic/elasticsearch/lib/Helpers.js:658:7)
at send (/app/node_modules/@elastic/elasticsearch/lib/Helpers.js:629:9)
at iterate (/app/node_modules/@elastic/elasticsearch/lib/Helpers.js:543:9)
at async XXX.YYY (/app/dist/XXX)
at async XXX.ZZZ (/app/dist/XXX)

Would you be able to help with this? I'm happy to create a new issues if you prefer.

mrickard commented 11 months ago

HI @bartosz347 ! I'm sorry to hear you're having trouble. We run tests on several versions*, including 7.13.0, and we're not seeing that error. Would you be able to provide code that reproduces this behavior?

bartosz347 commented 11 months ago

@mrickard Here is a sample code that is failing when NewRelic (v11.6.0) is enabled:

await elasticClient.helpers.bulk({
  datasource: [
    {
      amount: 123,
    },
  ],
  onDocument() {
    return [
      {
        update: { _index: 'xxx', _id: 'yyyyyy' },
      },
      { doc_as_upsert: true },
    ];
  },
});

Client docs

I'm running Elasticsearch locally with Docker image: elasticsearch:7.13.0 and and have @elastic/elasticsearch@7.13.0 package installed.

mrickard commented 11 months ago

@bartosz347 Thank you for the repro!

Were you successfully using our instrumentation prior to v11.6.0 of the agent?

workato-integration[bot] commented 11 months ago

https://new-relic.atlassian.net/browse/NR-187314

bartosz347 commented 11 months ago

Were you successfully using our instrumentation prior to v11.6.0 of the agent?

@mrickard No, we first tried to upgrade to v11.2.1, but due to ElasticSearch errors we've reverted to v11.1.0. This week I tried to upgrade to v11.6.0. Some ES queries started to work but not all of them.

mrickard commented 11 months ago

@bartosz347 Would you be able to open this as a new issue?

bartosz347 commented 11 months ago

@bartosz347 Would you be able to open this as a new issue?

Done – https://github.com/newrelic/node-newrelic/issues/1908