Skip to content

feat(observability): trace Database methods #2119

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 30, 2024
Merged

feat(observability): trace Database methods #2119

merged 1 commit into from
Sep 30, 2024

Conversation

odeke-em
Copy link
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 as code owners 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
Contributor Author

@surbhigarg92 kindly please re-review!

surbhigarg92
surbhigarg92 reviewed Sep 27, 2024
View reviewed changes
@odeke-em
Copy link
Contributor Author

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
Contributor Author

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

surbhigarg92
surbhigarg92 approved these changes Sep 30, 2024
View reviewed changes
Copy link
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

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
Contributor

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
Contributor Author

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!

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
Contributor

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
Contributor Author

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
Contributor Author

@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
16 checks passed
@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 join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@surbhigarg92 surbhigarg92 surbhigarg92 approved these changes

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