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

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 as a code owner Nov 3, 2020
@product-auto-label product-auto-label bot added the api: spanner label Nov 3, 2020
@google-cla google-cla bot added the cla: yes label Nov 3, 2020
@generated-files-bot

This comment has been minimized.

@skuruppu skuruppu requested a review from thiagotnunes Nov 3, 2020
@generated-files-bot

This comment has been minimized.

1 similar comment
@generated-files-bot

This comment has been minimized.

@mayurkale22
Copy link
Member Author

@mayurkale22 mayurkale22 commented Nov 3, 2020

+@syeduguri Please review

Copy link
Contributor

@thiagotnunes thiagotnunes left a comment

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 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 label Dec 30, 2020
@mayurkale22
Copy link
Member Author

@mayurkale22 mayurkale22 commented Dec 30, 2020

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

Copy link
Member Author

@mayurkale22 mayurkale22 left a comment

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

@mayurkale22 mayurkale22 removed the do not merge label Mar 31, 2021
@mayurkale22
Copy link
Member Author

@mayurkale22 mayurkale22 commented Mar 31, 2021

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

Copy link
Contributor

@thiagotnunes thiagotnunes left a comment

Just a few nits, but feel free to ignore them

* {@link AbstractReadContext} does not have a transaction tag.
*/
@Nullable
String getTransactionTag() {
Copy link
Contributor

@thiagotnunes thiagotnunes Mar 31, 2021

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

@mayurkale22 mayurkale22 commented Apr 1, 2021

The samples build failure is unrelated to this change

@mayurkale22 mayurkale22 merged commit 2a9086f into googleapis:master Apr 6, 2021
18 checks passed
@mayurkale22 mayurkale22 deleted the tagging_support branch Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants