Skip to content
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.

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

feat: add support for tagging #576

Merged
merged 12 commits into from Apr 6, 2021
Merged

feat: add support for tagging #576

merged 12 commits into from Apr 6, 2021

Conversation

mayurkale22
Copy link
Member

@mayurkale22 mayurkale22 commented Nov 3, 2020

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.

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

This comment has been minimized.

Copy link
Collaborator

@olavloite olavloite left a comment

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.

1 similar comment
@generated-files-bot

This comment has been minimized.

@mayurkale22
Copy link
Member Author

+@syeduguri Please review

Copy link
Contributor

@thiagotnunes thiagotnunes left a comment

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.

@generated-files-bot

This comment has been minimized.

@codecov
Copy link

codecov bot commented Dec 30, 2020

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 Files Coverage Δ Complexity Δ
.../com/google/cloud/spanner/AbstractReadContext.java 87.02% <100.00%> (+0.19%) 47.00 <1.00> (+3.00)
...rc/main/java/com/google/cloud/spanner/Options.java 92.85% <100.00%> (+0.59%) 92.00 <4.00> (+7.00)
...oogle/cloud/spanner/PartitionedDmlTransaction.java 83.33% <100.00%> (+0.72%) 18.00 <0.00> (+3.00)
...ain/java/com/google/cloud/spanner/SessionImpl.java 87.20% <100.00%> (+0.30%) 34.00 <0.00> (+3.00)
...om/google/cloud/spanner/TransactionRunnerImpl.java 86.51% <100.00%> (+0.27%) 10.00 <0.00> (ø)
...a/com/google/cloud/spanner/SessionPoolOptions.java 69.53% <0.00%> (ø) 18.00% <0.00%> (+1.00%)
...ain/java/com/google/cloud/spanner/SessionPool.java 89.32% <0.00%> (+0.19%) 73.00% <0.00%> (+1.00%)
...cloud/spanner/connection/ReadWriteTransaction.java 82.38% <0.00%> (+0.27%) 60.00% <0.00%> (+1.00%)
...m/google/cloud/spanner/connection/SpannerPool.java 89.47% <0.00%> (+1.57%) 33.00% <0.00%> (ø%)
.../google/cloud/spanner/AbstractLazyInitializer.java 100.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.

@mayurkale22 mayurkale22 added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Dec 30, 2020
@mayurkale22
Copy link
Member Author

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

Copy link
Collaborator

@olavloite olavloite left a comment

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.

Copy link
Member Author

@mayurkale22 mayurkale22 left a comment

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.

@mayurkale22 mayurkale22 removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 31, 2021
@mayurkale22
Copy link
Member Author

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

Copy link
Contributor

@thiagotnunes thiagotnunes left a comment

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

* {@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
Copy link
Member Author

The samples build failure is unrelated to this change

@mayurkale22 mayurkale22 merged commit 2a9086f into googleapis:master Apr 6, 2021
@mayurkale22 mayurkale22 deleted the tagging_support branch April 6, 2021 22:51
ansh0l pushed a commit to ansh0l/java-spanner that referenced this pull request Nov 10, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/java-spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants