[BEAM-11982] Java Spanner - Implement IO Request Count metrics #15493

benEng · 2021-09-10T17:29:48Z

Please add a meaningful description for your change here

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

`ValidatesRunner` compliance status (on master branch)

Lang	ULR	Twister2
Go	---	---
Java
Python	---	---
XLang		---

Examples testing status on various runners

Lang	ULR	Dataflow	Flink	Samza	Spark	Twister2
Go	---	---	---	---	---	---	---
Java	---		---	---	---	---	---
Python	---	---	---	---	---	---	---
XLang	---	---	---	---	---	---	---

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Go	Java	Python

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
Non-portable
Portable	---			---	---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

…trics

benEng · 2021-09-10T17:50:40Z

@ajamato Would you help me to review the progress on this? It is still pending to add queryName as label metrics, but I want to ask your opinion about it.
I added queryName to ReadOperation Builder

beam/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/ReadOperation.java

Line 99 in 63c0723

public ReadOperation withQueryName(String queryName) {

but at this point the ReadOperation is not accessible, only Transaction

beam/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/BatchSpannerRead.java

Line 161 in 63c0723

ServiceCallMetric serviceCallMetric =

My suggested approach is to add queryName to CreateTransaction Builder so I can extract queryName inside ReadFromPartitionFn process method

beam/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java

Line 753 in 63c0723

public abstract static class CreateTransaction

codecov · 2021-09-10T17:57:32Z

Codecov Report

Merging #15493 (6989863) into master (587f58e) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #15493      +/-   ##
==========================================
+ Coverage   83.76%   83.77%   +0.01%     
==========================================
  Files         444      444              
  Lines       60319    61075     +756     
==========================================
+ Hits        50528    51168     +640     
- Misses       9791     9907     +116

Impacted Files	Coverage Δ
.../python/apache_beam/testing/test_stream_service.py	`88.37% <0.00%> (-4.66%)`	⬇️
...che_beam/runners/interactive/interactive_runner.py	`90.74% <0.00%> (-1.86%)`	⬇️
...pache_beam/runners/interactive/interactive_beam.py	`76.11% <0.00%> (-0.66%)`	⬇️
...hon/apache_beam/runners/worker/bundle_processor.py	`93.64% <0.00%> (-0.13%)`	⬇️
.../python/apache_beam/portability/api/metrics_pb2.py	`100.00% <0.00%> (ø)`
...on/apache_beam/portability/api/metrics_pb2_urns.py	`100.00% <0.00%> (ø)`
...eam/runners/interactive/interactive_environment.py	`91.25% <0.00%> (+0.01%)`	⬆️
sdks/python/apache_beam/io/gcp/bigquery.py	`75.93% <0.00%> (+0.40%)`	⬆️
...runners/interactive/display/pcoll_visualization.py	`86.36% <0.00%> (+0.50%)`	⬆️
sdks/python/apache_beam/io/gcp/bigquery_tools.py	`88.74% <0.00%> (+2.04%)`	⬆️
... and 1 more

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 587f58e...6989863. Read the comment docs.

kileys · 2021-09-21T18:23:57Z

@ajamato Could you provide feedback? Thanks!

ajamato · 2021-09-21T20:26:27Z

...google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/BatchSpannerRead.java

-import com.google.cloud.spanner.ResultSet;
-import com.google.cloud.spanner.Struct;
-import com.google.cloud.spanner.TimestampBound;
+import com.google.cloud.spanner.*;


Could you run ./gradlew spotlessApply ?

I think the preferred style is to avoid wildcard imports

ajamato · 2021-09-21T20:26:47Z

...ava/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java

-import java.util.Comparator;
-import java.util.List;
-import java.util.OptionalInt;
+import java.util.*;


ditto, wildcard imports

ajamato · 2021-09-21T20:27:22Z

...oogle-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIOReadTest.java

-import com.google.cloud.spanner.TimestampBound;
-import com.google.cloud.spanner.Type;
-import com.google.cloud.spanner.Value;
+import com.google.cloud.spanner.*;


ditto, wildcard imports

kileys · 2021-09-24T00:33:59Z

Run Python PreCommit

kileys · 2021-09-24T18:26:55Z

@aaltay could you merge? Thanks!

benEng · 2021-09-27T14:26:05Z

Thank you all!

…e#15493)

nielm · 2022-01-14T12:33:53Z

Note: This PR makes the GCP projectID a required parameter - which it was not before, as it could be inferred from the environment - and thus breaks backward compatibility.

Specifically: BatchSpannerRead.java:175 -- the toString() is not required on the valueProvider, it should just be a get(), and createServiceCallMetric() in line 195 should handle the situation where projectID could be null

lukecwik · 2022-01-14T17:46:37Z

Thanks, filed https://issues.apache.org/jira/browse/BEAM-13665.

@nielm is this something you are interested in fixing?

nielm · 2022-01-18T16:16:15Z

Filed #16547

Fixes regression introduced by PR apache#15493 which inadvertently caused an NPE when the projectID was not specified for a SpannerIO read or write. Adds unit test for reading/writing both with and without projectID

Fixes regression introduced by PR #15493 which inadvertently caused an NPE when the projectID was not specified for a SpannerIO read or write. Adds unit test for reading/writing both with and without projectID

benEng added 9 commits August 27, 2021 17:56

[BEAM-11982] Add String resource and labels for SpannerIO requests me…

574dcf0

…trics

[BEAM-11982] Write Spanner Metrics

f3f39a8

Merge remote-tracking branch 'upstream/master' into BEAM-11982

efac793

[BEAM-11982] Add metrics to read spanner transform

74ae3fe

[BEAM-11982] Add Spanner Read Metrics test

3599b0d

[BEAM-11982] Add queryName to Read optional params

155f507

Merge master

40a3411

[BEAM-11982] Fix proto numbers

c30f061

[BEAM-11982] Fix spotlessApply

63c0723

ajamato approved these changes Sep 21, 2021

View reviewed changes

[BEAM-11982] Refactor imports

d2b9059

benEng changed the title ~~[WIP][BEAM-11982] Java Spanner - Implement IO Request Count metrics~~ [BEAM-11982] Java Spanner - Implement IO Request Count metrics Sep 21, 2021

Merge master

6989863

benEng marked this pull request as ready for review September 21, 2021 22:14

ajamato approved these changes Sep 23, 2021

View reviewed changes

aaltay merged commit 974d4e3 into apache:master Sep 25, 2021

dmitriikuzinepam pushed a commit to dmitriikuzinepam/beam that referenced this pull request Nov 2, 2021

[BEAM-11982] Java Spanner - Implement IO Request Count metrics (apach…

0842805

…e#15493)

nielm mentioned this pull request Jan 18, 2022

[BEAM-13665] Fix SpannerIO NPE when projectID is not specified #16547

Merged

[BEAM-11982] Java Spanner - Implement IO Request Count metrics #15493

[BEAM-11982] Java Spanner - Implement IO Request Count metrics #15493

Uh oh!

Conversation

benEng commented Sep 10, 2021

ValidatesRunner compliance status (on master branch)

Examples testing status on various runners

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

GitHub Actions Tests Status (on master branch)

Uh oh!

benEng commented Sep 10, 2021

Uh oh!

codecov bot commented Sep 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kileys commented Sep 21, 2021

Uh oh!

ajamato Sep 21, 2021

Choose a reason for hiding this comment

Uh oh!

benEng Sep 21, 2021

Choose a reason for hiding this comment

Uh oh!

ajamato Sep 21, 2021

Choose a reason for hiding this comment

Uh oh!

ajamato Sep 21, 2021

Choose a reason for hiding this comment

Uh oh!

kileys commented Sep 24, 2021

Uh oh!

kileys commented Sep 24, 2021

Uh oh!

benEng commented Sep 27, 2021

Uh oh!

nielm commented Jan 14, 2022

Uh oh!

lukecwik commented Jan 14, 2022

Uh oh!

nielm commented Jan 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

`ValidatesRunner` compliance status (on master branch)

codecov bot commented Sep 10, 2021 •

edited

Loading