[BEAM-11982] Java Spanner - Implement IO Request Count metrics by benEng · Pull Request #15493 · apache/beam

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

publicReadOperationwithQueryName(StringqueryName) {

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

ServiceCallMetricserviceCallMetric =

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

publicabstractstaticclassCreateTransaction

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

-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

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


ditto, wildcard imports

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

-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 metricsSep 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:masterSep 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

Navigation Menu

Uh oh!

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

[BEAM-11982] Java Spanner - Implement IO Request Count metrics#15493
aaltay merged 11 commits into
apache:masterfrom
benEng:BEAM-11982

benEng commented Sep 10, 2021

Uh oh!

benEng commented Sep 10, 2021

Uh oh!

codecovBot commented Sep 10, 2021•
edited
Loading

Uh oh!

kileys commented Sep 21, 2021

Uh oh!

ajamatoSep 21, 2021

Uh oh!

benEngSep 21, 2021

Uh oh!

ajamatoSep 21, 2021

Uh oh!

ajamatoSep 21, 2021

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

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!

codecovBot 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!

ajamatoSep 21, 2021

Choose a reason for hiding this comment

Uh oh!

benEngSep 21, 2021

Choose a reason for hiding this comment

Uh oh!

ajamatoSep 21, 2021

Choose a reason for hiding this comment

Uh oh!

ajamatoSep 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)

codecovBot commented Sep 10, 2021•
edited
Loading