fix: Client side metrics should record the version number of @google-cloud/bigtable not the customer package version

[danieljbruce]

Description

A bug was discovered where the client side metrics being recorded on the backend service had the wrong version number. The version number being recorded actually matched the version number on the customer's package.json file instead of the version number of the Node Bigtable package being used.

To correct this, the line of code at

nodejs-bigtable/src/client-side-metrics/operation-metrics-collector.ts

Line 52 in 2a96092

const packageJSON = fs.readFileSync('package.json');

which is currently reading the package.json file relative to the working directory of the node process should instead read the package.json file location relative to the location of operation-metrics-collector.ts so that the version number of the Node Bigtable dependency is always recorded instead of the version number from the customer's package.json file.

Impact

Corrects the bug and ensures that the right version number is always recorded for client side metrics.

Files affected

samples/readSnippets: A code snippet that will run in the samples test and fail if the metrics handler receives the wrong version of the client library

samples/test/reads.js: The new samples test that runs the code snippet in readSnippets and fails if the metrics handler receives the wrong version of the client library

src/client-side-metrics/operation-metrics-collector.ts: The source code changes that ensure the version number recorded will always match the Bigtable client library version number

test-common/test-metrics-handler.ts: A new class, similar to the existing class for testing data sent to the metrics handler that also preserves the version name when it reaches the metrics handler

Testing

A new system test or unit test will not catch this bug since their working directories all reference the package.json file in the nodejs-bigtable project. So instead, I created a fake customer project with the script from https://gist.github.com/danieljbruce/bba73fa5d9919866d82b8968ceb45cc3 which gets the wrong version number at

nodejs-bigtable/src/client-side-metrics/operation-metrics-collector.ts

Line 52 in 2a96092

const packageJSON = fs.readFileSync('package.json');

, but gets the right version number when this line of code is corrected with the changes in this PR.

I also found a way to create a samples test which will fail without the source code changes from this PR. It should ensure going forward that the right version gets recorded for client side metrics.

[snippet-bot]

No region tags are edited in this PR.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

• Refresh this comment

[daniel-sanche]

samples/readSnippets: A code snippet that will run in the samples test and fail if the metrics handler receives the wrong version of the client library

I'm a bit confused by this. Region-tagged samples are meant to be pulled into the docs, to show users how to interact with the library. This description, and the sample itself seems more like it's meant to be a unit test?

[daniel-sanche]

Gemini says you can do const { version } = require('../../../package.json'); or import { version } from '../../../package.json';

I don't know enough about node to know if that's good advice

[danieljbruce]

Yeah. I think this is good so I made the change. It eliminates the fs dependency and it is shorter.

[danieljbruce]

samples/readSnippets: A code snippet that will run in the samples test and fail if the metrics handler receives the wrong version of the client library

I'm a bit confused by this. Region-tagged samples are meant to be pulled into the docs, to show users how to interact with the library. This description, and the sample itself seems more like it's meant to be a unit test?

You're intuition is right. The test that was added is meant to be a unit test. We are not adding a new sample here and have no intention of adding a sample to the docs.

The main problem with adding a new system-test or unit test is that as you can see in

nodejs-bigtable/package.json

Lines 41 to 44 in 2a96092

	"system-test": "mocha build/system-test --timeout 600000",
	"pretest": "npm run compile",
	"test": "c8 mocha build/test",
	"test:snap": "SNAPSHOT_UPDATE=1 npm test",

, these processes spawn from the root directory of nodejs-bigtable. So this means that even with code before this PR, a system test or unit test will always read the package.json version from package.json file in nodejs-bigtable root and incorrectly produce a passing result when the test should actually fail if simulating customer code correctly.

That said, maybe the test will fail properly as a unit test if I spawn a subprocess using execSync like the samples tests so I'll give that a try.

[danieljbruce]

samples/readSnippets: A code snippet that will run in the samples test and fail if the metrics handler receives the wrong version of the client library

I'm a bit confused by this. Region-tagged samples are meant to be pulled into the docs, to show users how to interact with the library. This description, and the sample itself seems more like it's meant to be a unit test?

You're intuition is right. The test that was added is meant to be a unit test. We are not adding a new sample here and have no intention of adding a sample to the docs.

The main problem with adding a new system-test or unit test is that as you can see in

nodejs-bigtable/package.json

Lines 41 to 44 in 2a96092

	"system-test": "mocha build/system-test --timeout 600000",
	"pretest": "npm run compile",
	"test": "c8 mocha build/test",
	"test:snap": "SNAPSHOT_UPDATE=1 npm test",

, these processes spawn from the root directory of nodejs-bigtable. So this means that even with code before this PR, a system test or unit test will always read the package.json version from package.json file in nodejs-bigtable root and incorrectly produce a passing result when the test should actually fail if simulating customer code correctly.
That said, maybe the test will fail properly as a unit test if I spawn a subprocess using execSync like the samples tests so I'll give that a try.

Alright. I tried creating a unit test and got it to fail when it should after wrestling with some linting issues and other errors. PTAL @daniel-sanche

[daniel-sanche]

LGTM