Skip to content
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

Crash in elasticsearch instrumentation with null body. #1809

Closed
villelahdenvuo opened this issue Oct 13, 2023 · 19 comments · Fixed by #1816
Closed

Crash in elasticsearch instrumentation with null body. #1809

villelahdenvuo opened this issue Oct 13, 2023 · 19 comments · Fixed by #1816
Assignees
Labels

Comments

@villelahdenvuo
Copy link

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
Copy link

@mrickard
Copy link
Member

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

@villelahdenvuo
Copy link
Author

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

@bizob2828
Copy link
Member

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

@villelahdenvuo
Copy link
Author

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

@bizob2828
Copy link
Member

@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
Copy link
Author

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

@bizob2828
Copy link
Member

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

@villelahdenvuo
Copy link
Author

villelahdenvuo commented Oct 13, 2023

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
Copy link
Member

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

@mrickard
Copy link
Member

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

@bartosz347
Copy link

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
Copy link
Member

mrickard commented Dec 7, 2023

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?

  • Current passing versions are 7.10.0, 7.13.0, 7.14.1, 7.16.0, 8.0.0, 8.1.0, 8.4.0, 8.6.0, 8.7.0, 8.9.1, 8.10.0.

@bartosz347
Copy link

@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
Copy link
Member

mrickard commented Dec 7, 2023

@bartosz347 Thank you for the repro!

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

@mrickard mrickard reopened this Dec 7, 2023
@workato-integration
Copy link

@bartosz347
Copy link

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
Copy link
Member

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

@bartosz347
Copy link

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

Done – #1908

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

Successfully merging a pull request may close this issue.

4 participants