Skip to content
This repository was archived by the owner on Mar 4, 2026. It is now read-only.

feat(observability): trace Database methods#2119

Merged
gcf-merge-on-green[bot] merged 1 commit intogoogleapis:mainfrom
odeke-em:trace-Database
Sep 30, 2024
Merged

feat(observability): trace Database methods#2119
gcf-merge-on-green[bot] merged 1 commit intogoogleapis:mainfrom
odeke-em:trace-Database

Conversation

@odeke-em
Copy link
Copy Markdown
Contributor

@odeke-em odeke-em commented Sep 20, 2024 •
edited
Loading

Adds trace spans for Database methods, as well as tests
for methods:

  • getSession
  • getSnapshot
  • run
  • runStream
  • runTransaction

tracing of other methods shall come in follow-up PRs.

Updates #2079
Built from PR #2087
Updates #2114

@odeke-em odeke-em requested review from a team September 20, 2024 10:17
@product-auto-label product-auto-label Bot added size: xl Pull request size is extra large. api: spanner Issues related to the googleapis/nodejs-spanner API. labels Sep 20, 2024
@odeke-em odeke-em force-pushed the trace-Database branch 2 times, most recently from 87ebbf7 to c6e430f Compare September 20, 2024 11:06
@product-auto-label product-auto-label Bot added size: l Pull request size is large. and removed size: xl Pull request size is extra large. labels Sep 20, 2024
@odeke-em odeke-em force-pushed the trace-Database branch 3 times, most recently from 65ca9f3 to 548d980 Compare September 23, 2024 10:27
@product-auto-label product-auto-label Bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Sep 27, 2024
@odeke-em odeke-em force-pushed the trace-Database branch 3 times, most recently from 204b67a to 186ed39 Compare September 27, 2024 09:22
@odeke-em
Copy link
Copy Markdown
Contributor Author

odeke-em commented Sep 27, 2024

@surbhigarg92 kindly please re-review!

surbhigarg92
surbhigarg92 reviewed Sep 27, 2024
View reviewed changes
Comment thread test/database.ts Outdated
Comment thread test/spanner.ts Outdated
Comment thread observability-test/spanner.ts
Comment thread observability-test/database.ts Outdated
@odeke-em
Copy link
Copy Markdown
Contributor Author

odeke-em commented Sep 29, 2024

Hello @surbhigarg92 kindly please take a look. Thank you!

@odeke-em odeke-em force-pushed the trace-Database branch 4 times, most recently from 4c1aa52 to f4f4565 Compare September 30, 2024 03:28
@odeke-em
Copy link
Copy Markdown
Contributor Author

odeke-em commented Sep 30, 2024

@surbhigarg92 @harshachinta kindly please help trigger the bot runs. Thank you.

surbhigarg92
surbhigarg92 approved these changes Sep 30, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@surbhigarg92 surbhigarg92 left a comment

Choose a reason for hiding this comment

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

Please fix the 2 comments before merging

Comment thread test/database.ts Outdated
// depending on the version of Node.js, the error might be either of:
// * Cannot read properties of null (reading 'proto')
// * Cannot read property 'proto' of null
(err as grpc.ServiceError).message.includes('Cannot read propert');
Copy link
Copy Markdown
Contributor

@surbhigarg92 surbhigarg92 Sep 30, 2024

Choose a reason for hiding this comment

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

instead of this do something like
const errorMessage = (err as grpc.ServiceError).message;
errorMessage.includes("Cannot read properties of null (reading 'proto')") ||
errorMessage.includes("Cannot read property 'proto' of null")

Copy link
Copy Markdown
Contributor Author

@odeke-em odeke-em Sep 30, 2024

Choose a reason for hiding this comment

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

Awesome and thank you @surbhigarg92!

Comment thread test/database.ts
assert.strictEqual(runFn, fakeRunFn);
// Given that we've wrapped the transaction runner with observability
// tracing, directly comparing values runFn and fakeRunFn.
// assert.strictEqual(runFn, fakeRunFn);
Copy link
Copy Markdown
Contributor

@surbhigarg92 surbhigarg92 Sep 30, 2024

Choose a reason for hiding this comment

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

is it possible to add a check on some properties of these functions, instead of removing this check ?

Copy link
Copy Markdown
Contributor Author

@odeke-em odeke-em Sep 30, 2024

Choose a reason for hiding this comment

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

Sadly not, unless we are perform a traversal by utility examination but I found that to be overkill and not worth it TBH

@odeke-em
feat(observability): trace Database methods
eeb628b 
Adds trace spans for Database methods, as well as tests
for methods:

* getSession
* getSnapshot
* run
* runStream
* runTransaction

tracing of other methods shall come in follow-up PRs.

Updates googleapis#2079
Built from PR googleapis#2087
Updates googleapis#2114
@odeke-em
Copy link
Copy Markdown
Contributor Author

odeke-em commented Sep 30, 2024

@surbhigarg92 thank you very much for the reviews! I've addressed all feedback and gotten the PR ready for merge by squashing commits into one. Kindly help me re-run the bots.

@surbhigarg92 surbhigarg92 added automerge Merge the pull request once unit tests and other checks pass. kokoro:run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Sep 30, 2024
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 30, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Sep 30, 2024
@gcf-merge-on-green gcf-merge-on-green Bot merged commit 1f06871 into googleapis:main Sep 30, 2024
@gcf-merge-on-green gcf-merge-on-green Bot removed the automerge Merge the pull request once unit tests and other checks pass. label Sep 30, 2024
@release-please release-please Bot mentioned this pull request Sep 29, 2024
Merged
@odeke-em odeke-em deleted the trace-Database branch September 30, 2024 08:07
gcf-merge-on-green Bot pushed a commit that referenced this pull request Oct 30, 2024
@release-please
chore(main): release 7.15.0 (#2118)
be063b0 
🤖 I have created a release *beep* *boop*
---


## [7.15.0](https://togithub.com/googleapis/nodejs-spanner/compare/v7.14.0...v7.15.0) (2024-10-30)


### Features

* (observability, samples): add tracing end-to-end sample ([#2130](https://togithub.com/googleapis/nodejs-spanner/issues/2130)) ([66d99e8](https://togithub.com/googleapis/nodejs-spanner/commit/66d99e836cd2bfbb3b0f78980ec2b499f9e5e563))
* (observability) add spans for BatchTransaction and Table ([#2115](https://togithub.com/googleapis/nodejs-spanner/issues/2115)) ([d51aae9](https://togithub.com/googleapis/nodejs-spanner/commit/d51aae9c9c3c0e6319d81c2809573ae54675acf3)), closes [#2114](https://togithub.com/googleapis/nodejs-spanner/issues/2114)
* (observability) Add support for OpenTelemetry traces and allow observability options to be passed.  ([#2131](https://togithub.com/googleapis/nodejs-spanner/issues/2131)) ([5237e11](https://togithub.com/googleapis/nodejs-spanner/commit/5237e118befb4b7fe4aea76a80a91e822d7a22e4)), closes [#2079](https://togithub.com/googleapis/nodejs-spanner/issues/2079)
* (observability) propagate database name for every span generated to aid in quick debugging ([#2155](https://togithub.com/googleapis/nodejs-spanner/issues/2155)) ([0342e74](https://togithub.com/googleapis/nodejs-spanner/commit/0342e74721a0684d8195a6299c3a634eefc2b522))
* (observability) trace Database.batchCreateSessions + SessionPool.createSessions ([#2145](https://togithub.com/googleapis/nodejs-spanner/issues/2145)) ([f489c94](https://togithub.com/googleapis/nodejs-spanner/commit/f489c9479fa5402f0c960cf896fd3be0e946f182))
* (observability): trace Database.runPartitionedUpdate ([#2176](https://togithub.com/googleapis/nodejs-spanner/issues/2176)) ([701e226](https://togithub.com/googleapis/nodejs-spanner/commit/701e22660d5ac9f0b3e940ad656b9ca6c479251d)), closes [#2079](https://togithub.com/googleapis/nodejs-spanner/issues/2079)
* (observability): trace Database.runTransactionAsync ([#2167](https://togithub.com/googleapis/nodejs-spanner/issues/2167)) ([d0fe178](https://togithub.com/googleapis/nodejs-spanner/commit/d0fe178623c1c48245d11bcea97fcd340b6615af)), closes [#207](https://togithub.com/googleapis/nodejs-spanner/issues/207)
* Allow multiple KMS keys to create CMEK database/backup ([#2099](https://togithub.com/googleapis/nodejs-spanner/issues/2099)) ([51bc8a7](https://togithub.com/googleapis/nodejs-spanner/commit/51bc8a7445ab8b3d2239493b69d9c271c1086dde))
* **observability:** Fix bugs found from product review + negative cases ([#2158](https://togithub.com/googleapis/nodejs-spanner/issues/2158)) ([cbc86fa](https://togithub.com/googleapis/nodejs-spanner/commit/cbc86fa80498af6bd745eebb9443612936e26d4e))
* **observability:** Trace Database methods ([#2119](https://togithub.com/googleapis/nodejs-spanner/issues/2119)) ([1f06871](https://togithub.com/googleapis/nodejs-spanner/commit/1f06871f7aca386756e8691013602b069697bb87)), closes [#2114](https://togithub.com/googleapis/nodejs-spanner/issues/2114)
* **observability:** Trace Database.batchWriteAtLeastOnce ([#2157](https://togithub.com/googleapis/nodejs-spanner/issues/2157)) ([2a19ef1](https://togithub.com/googleapis/nodejs-spanner/commit/2a19ef1af4f6fd1b81d08afc15db76007859a0b9)), closes [#2079](https://togithub.com/googleapis/nodejs-spanner/issues/2079)
* **observability:** Trace Transaction ([#2122](https://togithub.com/googleapis/nodejs-spanner/issues/2122)) ([a464bdb](https://togithub.com/googleapis/nodejs-spanner/commit/a464bdb5cbb7856b7a08dac3ff48132948b65792)), closes [#2114](https://togithub.com/googleapis/nodejs-spanner/issues/2114)


### Bug Fixes

* Exact staleness timebound ([#2143](https://togithub.com/googleapis/nodejs-spanner/issues/2143)) ([f01516e](https://togithub.com/googleapis/nodejs-spanner/commit/f01516ec6ba44730622cfb050c52cd93f30bba7a)), closes [#2129](https://togithub.com/googleapis/nodejs-spanner/issues/2129)
* GetMetadata for Session ([#2124](https://togithub.com/googleapis/nodejs-spanner/issues/2124)) ([2fd63ac](https://togithub.com/googleapis/nodejs-spanner/commit/2fd63acb87ce06a02d7fdfa78d836dbd7ad59a26)), closes [#2123](https://togithub.com/googleapis/nodejs-spanner/issues/2123)

---
This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@surbhigarg92 surbhigarg92 surbhigarg92 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: spanner Issues related to the googleapis/nodejs-spanner API. size: xl Pull request size is extra large.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@odeke-em @surbhigarg92 @yoshi-kokoro