elastic / apm-agent-nodejs

https://www.elastic.co/guide/en/apm/agent/nodejs/current/index.html
BSD 2-Clause "Simplified" License
582 stars 225 forks source link

Guard against `cassandra-driver` incompatible versions #3629

Closed david-luna closed 1 year ago

david-luna commented 1 year ago

Version 4.7.0 of cassandra-driver is using Object.hasOwn API which is supported from node v16.9.0. The engines.node property of its package.json file hasn't changed though and allows versions down to node 8

The agent is testing in from nodejs v14 so we got failing tests when testing cassandra-driver on node 14.x or 16.0 like it happens on this PR https://github.com/elastic/apm-agent-nodejs/pull/3626

We should:

NOTE to self: be a good boy scout and revisit the tests (runTestFixtures FTW)

trentm commented 1 year ago

We should perhaps open an issue on https://github.com/datastax/nodejs-driver/ for this.

david-luna commented 1 year ago

Issue link: https://datastax-oss.atlassian.net/browse/NODEJS-665

david-luna commented 1 year ago

Versioning issues on testing and new tests using runTestFixtures where added in https://github.com/elastic/apm-agent-nodejs/pull/3626

cassandra-driver@4.7.1 set the node version to v16 although the author left a comment telling it works on previous versions. The reason for the bump is to drop versions that are in EOL (making v16 an exception) Link to comment: https://github.com/datastax/nodejs-driver/pull/415#discussion_r1331010721

I guess we should not instrument the newest versions if node version is lower than 16. WDYT @trentm ?

trentm commented 1 year ago

Checking versions again:

So:

  1. I don't have a strong opinion, but given their comment, I am inclined to still instrument cassandra-driver@4 with node 16.0.0 and 14.x because that comment suggests it might work. Let's not cut off the possible user of cassandra-driver: 4.x who is still using old node v16 or node v14.
  2. However, let's only bother testing our cassandra-driver instrumentation with node v16.9.0 forward because (a) we want to test node v16.<something> at least and (b) specifically having a guard against the "bad" 4.7.0 and 4.7.1 releases is a somewhat of a pain.

My opinion on (1) isn't strong. I'd also be fine with the argument to not instrument cassandra-driver >=4.7.2 with node <16 based on their "engines" setting.

david-luna commented 1 year ago

Let's not cut off the possible user of cassandra-driver: 4.x who is still using old node v16 or node v14.

It makes sense to let the agent run if the combo cassndra/node allows it. Current instrumentation only checks for cassandra version so there is no need to apply any change.

About (2) we have the tests split in this way:

But I agree that it's quite tricky. Let's simplify it in a PR :)