Conversation

mayurkale22

Description

Add support to enable users to set tags on database operations such as reads, queries and transactions.

Background

The statistics of queries, reads and transactions in Cloud Spanner are stored. Providing tags for database operations will allow these statistics to be grouped by a tag and makes queries/transactions easily searchable by tag. This will help make the information provided by the statistics more useful.

TODO

  • Rebase when RequestOptions proto changes are published.

Note: samples will be handled in a separate PR.

@mayurkale22mayurkale22 requested a review from a team as a code owner November 3, 2020 05:37
@product-auto-labelproduct-auto-label bot added the api: spannerIssues related to the googleapis/java-spanner API.label Nov 3, 2020
@google-clagoogle-cla bot added the cla: yesThis human has signed the Contributor License Agreement.label Nov 3, 2020
@generated-files-bot

This comment has been minimized.

Sign in to view
olavloite
olavloite reviewed Nov 3, 2020
View reviewed changes

Choose a reason for hiding this comment

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

This approach looks generally good to me.

@generated-files-bot

This comment has been minimized.

Sign in to view
1 similar comment
@generated-files-bot

This comment has been minimized.

Sign in to view
@mayurkale22

+@syeduguri Please review

thiagotnunes
thiagotnunes reviewed Nov 3, 2020
View reviewed changes

Choose a reason for hiding this comment

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

Looks good to me overall

@generated-files-bot

This comment has been minimized.

Sign in to view
@olavloiteolavloite mentioned this pull request Nov 13, 2020
Merged
@olavloiteolavloite mentioned this pull request Dec 1, 2020
Merged
@olavloiteolavloite mentioned this pull request Dec 11, 2020
Merged
@generated-files-bot

This comment has been minimized.

Sign in to view
@codecov

Codecov Report

Merging #576 (6bb97a4) into master (0bc9972) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #576      +/-   ##
============================================
+ Coverage     85.18%   85.27%   +0.08%     
- Complexity     2640     2660      +20     
============================================
  Files           155      155              
  Lines         14413    14449      +36     
  Branches       1348     1362      +14     
============================================
+ Hits          12278    12321      +43     
+ Misses         1567     1561       -6     
+ Partials        568      567       -1
Impacted FilesCoverage ΔComplexity Δ
.../com/google/cloud/spanner/AbstractReadContext.java87.02% <100.00%> (+0.19%)47.00 <1.00> (+3.00)
...rc/main/java/com/google/cloud/spanner/Options.java92.85% <100.00%> (+0.59%)92.00 <4.00> (+7.00)
...oogle/cloud/spanner/PartitionedDmlTransaction.java83.33% <100.00%> (+0.72%)18.00 <0.00> (+3.00)
...ain/java/com/google/cloud/spanner/SessionImpl.java87.20% <100.00%> (+0.30%)34.00 <0.00> (+3.00)
...om/google/cloud/spanner/TransactionRunnerImpl.java86.51% <100.00%> (+0.27%)10.00 <0.00> (ø)
...a/com/google/cloud/spanner/SessionPoolOptions.java69.53% <0.00%> (ø)18.00% <0.00%> (+1.00%)
...ain/java/com/google/cloud/spanner/SessionPool.java89.32% <0.00%> (+0.19%)73.00% <0.00%> (+1.00%)
...cloud/spanner/connection/ReadWriteTransaction.java82.38% <0.00%> (+0.27%)60.00% <0.00%> (+1.00%)
...m/google/cloud/spanner/connection/SpannerPool.java89.47% <0.00%> (+1.57%)33.00% <0.00%> (ø%)
.../google/cloud/spanner/AbstractLazyInitializer.java100.00% <0.00%> (+7.14%)5.00% <0.00%> (+1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0bc9972...6bb97a4. Read the comment docs.

@mayurkale22mayurkale22 added the do not mergeIndicates a pull request not ready for merge, due to either quality or timing.label Dec 30, 2020
@mayurkale22

@olavloite @thiagotnunes I re-rebased PR based on #716, I'd like to get your reviews and approval.

olavloite
olavloite requested changes Dec 31, 2020
View reviewed changes

Choose a reason for hiding this comment

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

I think the transaction tag feature could be simplified somewhat. See my suggestions below.

mayurkale22
mayurkale22 commented Jan 1, 2021
View reviewed changes

Choose a reason for hiding this comment

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

@olavloite thanks for the suggestions. I made the necessary changes, PTAL.

@vi3k6i5vi3k6i5 mentioned this pull request Mar 5, 2021
Closed
@mayurkale22mayurkale22 removed the do not mergeIndicates a pull request not ready for merge, due to either quality or timing.label Mar 31, 2021
@mayurkale22

@thiagotnunes @elharo @skuruppu this is ready to review. PTAL

thiagotnunes
thiagotnunes approved these changes Mar 31, 2021
View reviewed changes

Choose a reason for hiding this comment

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

Just a few nits, but feel free to ignore them

google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractReadContext.java
* {@link AbstractReadContext} does not have a transaction tag.
*/
@Nullable
String getTransactionTag() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could we also create a hasTransactionTag() that checks if getTransactionTag() != null?

@mayurkale22

The samples build failure is unrelated to this change

elharo
elharo reviewed Apr 1, 2021
View reviewed changes
@mayurkale22mayurkale22 merged commit 2a9086f into googleapis:master Apr 6, 2021
@mayurkale22mayurkale22 deleted the tagging_support branch April 6, 2021 22:51
@mayurkale22mayurkale22 mentioned this pull request Apr 7, 2021
Closed
@mayurkale22mayurkale22 mentioned this pull request Sep 30, 2021
Closed
4 tasks
ansh0l pushed a commit to ansh0l/java-spanner that referenced this pull request Nov 10, 2022
@yoshi-automation
chore(all): auto-regenerate .pb.go files (googleapis#576)
64678f1
This is an auto-generated regeneration of the .pb.go files by
cloud.google.com/go/internal/gapicgen. Once this PR is submitted, genbot will
update the corresponding PR to depend on the newer version of go-genproto, and
assign reviewers. Whilst this or any regen PR is open in go-genproto, genbot
will not create any more regeneration PRs. If all regen PRs are closed,
gapicgen will create a new set of regeneration PRs once per night.

If you have been assigned to review this PR, please:

- Ensure that CI is passing. If it's failing, it requires your manual attention.
- Approve and submit this PR if you believe it's ready to ship. That will prompt
genbot to assign reviewers to the google-cloud-go PR.

Corresponding google-cloud-go PR: googleapis/google-cloud-go#3943

Changes:

feat(cloudbuild/apiv1): Add fields for Pub/Sub triggers
  Committer: @gleeper
  PiperOrigin-RevId: 368533270
  Source-Link: googleapis/googleapis@9a9e296

fix: configuring timeouts for aiplatform v1 methods
  PiperOrigin-RevId: 368532697
  Source-Link: googleapis/googleapis@a8615e2
rajatbhatta pushed a commit to rajatbhatta/java-spanner that referenced this pull request Nov 17, 2022
@release-please
chore: release 2.4.0 (googleapis#576)
16cb4e3
🤖 I have created a release \*beep\* \*boop\*
---
## [2.4.0](https://www..com/googleapis/java-spanner-jdbc/compare/v2.3.5...v2.4.0) (2021-08-27)


### Features

* support JSON data type ([googleapis#447](https://www..com/googleapis/java-spanner-jdbc/issues/447)) ([ca1c906](https://www..com/googleapis/java-spanner-jdbc/commit/ca1c906e1ed3cad6444068ab9c8465401d6d3074))


### Dependencies

* update dependency com.google.cloud:google-cloud-spanner-bom to v6.12.1 ([googleapis#577](https://www..com/googleapis/java-spanner-jdbc/issues/577)) ([a78b177](https://www..com/googleapis/java-spanner-jdbc/commit/a78b177f97c298a5b43fcadbca125e957e9f781a))
---


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

@elharo elharo elharo left review comments

@thiagotnunes thiagotnunes thiagotnunes approved these changes

@olavloite olavloite olavloite approved these changes

@syeduguri syeduguri Awaiting requested review from syeduguri

No one assigned
api: spannerIssues related to the googleapis/java-spanner API.cla: yesThis human has signed the Contributor License Agreement.
None yet
No milestone

Successfully merging this pull request may close these issues.