You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2021/07/20 19:02:39 UTC
[GitHub] [beam] ajamato commented on a change in pull request #15183: [BEAM-11983] Java Datastore - Implement IO Request Count metrics
ajamato commented on a change in pull request #15183:
URL: https://github.com/apache/beam/pull/15183#discussion_r673398040
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/datastore/DatastoreV1.java
##########
@@ -901,12 +907,28 @@ private RunQueryResponse runQueryWithRetries(RunQueryRequest request) throws Exc
Sleeper sleeper = Sleeper.DEFAULT;
BackOff backoff = RUNQUERY_BACKOFF.backoff();
while (true) {
+ HashMap<String, String> baseLabels = new HashMap<>();
+ baseLabels.put(MonitoringInfoConstants.Labels.PTRANSFORM, "TODO");
+ baseLabels.put(MonitoringInfoConstants.Labels.SERVICE, "Datastore");
+ baseLabels.put(MonitoringInfoConstants.Labels.METHOD, "runQueryWithRetries");
+ baseLabels.put(MonitoringInfoConstants.Labels.RESOURCE, "TODO");
Review comment:
Please add a helper to gen the resource name for Datastore Namespaces
PTAL at the linked doc in
https://issues.apache.org/jira/browse/BEAM-11983
Simply build and populate a format string
RESOURCE -
Datastore Namespace:
"//bigtable.googleapis.com/projects/{projectId}/namespaces/{namespaceId}"
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/datastore/DatastoreV1.java
##########
@@ -919,6 +941,15 @@ private RunQueryResponse runQueryWithRetries(RunQueryRequest request) throws Exc
}
}
+ protected GoogleJsonError.ErrorInfo getErrorInfo(Exception e) {
+ if (!(e instanceof GoogleJsonResponseException)) {
+ return null;
+ }
+ GoogleJsonError jsonError = ((GoogleJsonResponseException) e).getDetails();
+ GoogleJsonError.ErrorInfo errorInfo = Iterables.getFirst(jsonError.getErrors(), null);
+ return errorInfo;
+ }
+
Review comment:
This PR only instruments reading from Datastore, but not writing.
Please instrument here as well. (You can do in a separate PR if you like, if so rename this PR to
"[BEAM-11983] Java Datastore - Implement IO Request Count metrics for Datastore Reads"
https://github.com/apache/beam/blob/fc24debff70dba842757dc249a7362abbdb34c75/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/datastore/DatastoreV1.java#L1341
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/datastore/DatastoreV1.java
##########
@@ -901,12 +907,28 @@ private RunQueryResponse runQueryWithRetries(RunQueryRequest request) throws Exc
Sleeper sleeper = Sleeper.DEFAULT;
BackOff backoff = RUNQUERY_BACKOFF.backoff();
while (true) {
+ HashMap<String, String> baseLabels = new HashMap<>();
+ baseLabels.put(MonitoringInfoConstants.Labels.PTRANSFORM, "TODO");
Review comment:
Do not populate with TODO, populate with ""
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/datastore/DatastoreV1.java
##########
@@ -901,12 +907,28 @@ private RunQueryResponse runQueryWithRetries(RunQueryRequest request) throws Exc
Sleeper sleeper = Sleeper.DEFAULT;
BackOff backoff = RUNQUERY_BACKOFF.backoff();
while (true) {
+ HashMap<String, String> baseLabels = new HashMap<>();
+ baseLabels.put(MonitoringInfoConstants.Labels.PTRANSFORM, "TODO");
+ baseLabels.put(MonitoringInfoConstants.Labels.SERVICE, "Datastore");
+ baseLabels.put(MonitoringInfoConstants.Labels.METHOD, "runQueryWithRetries");
+ baseLabels.put(MonitoringInfoConstants.Labels.RESOURCE, "TODO");
+ baseLabels.put(
+ MonitoringInfoConstants.Labels.BIGQUERY_PROJECT_ID, request.getProjectId());
Review comment:
Please add new labels for Datastore fields and use this. Do not use the BIQUERY_PROJECT_ID field
PTAL at the linked doc in
https://issues.apache.org/jira/browse/BEAM-11983
add labels for DATASTORE to the proro and populate them:
DATASTORE_PROJECT - Datastore project being read/written to
DATASTORE_NAMESPACE - Datastore namespace being read/written to
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/datastore/DatastoreV1.java
##########
@@ -901,12 +907,28 @@ private RunQueryResponse runQueryWithRetries(RunQueryRequest request) throws Exc
Sleeper sleeper = Sleeper.DEFAULT;
BackOff backoff = RUNQUERY_BACKOFF.backoff();
while (true) {
+ HashMap<String, String> baseLabels = new HashMap<>();
Review comment:
Please add test case to check if the metric is set. You can find examples in this PR:
https://github.com/apache/beam/pull/14501/files
see verifyWriteMetricWasSet
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/datastore/DatastoreV1.java
##########
@@ -901,12 +907,28 @@ private RunQueryResponse runQueryWithRetries(RunQueryRequest request) throws Exc
Sleeper sleeper = Sleeper.DEFAULT;
BackOff backoff = RUNQUERY_BACKOFF.backoff();
while (true) {
+ HashMap<String, String> baseLabels = new HashMap<>();
+ baseLabels.put(MonitoringInfoConstants.Labels.PTRANSFORM, "TODO");
+ baseLabels.put(MonitoringInfoConstants.Labels.SERVICE, "Datastore");
+ baseLabels.put(MonitoringInfoConstants.Labels.METHOD, "runQueryWithRetries");
+ baseLabels.put(MonitoringInfoConstants.Labels.RESOURCE, "TODO");
+ baseLabels.put(
+ MonitoringInfoConstants.Labels.BIGQUERY_PROJECT_ID, request.getProjectId());
+ ServiceCallMetric serviceCallMetric =
+ new ServiceCallMetric(MonitoringInfoConstants.Urns.API_REQUEST_COUNT, baseLabels);
try {
RunQueryResponse response = datastore.runQuery(request);
+ serviceCallMetric.call("ok");
rpcSuccesses.inc();
return response;
} catch (DatastoreException exception) {
rpcErrors.inc();
+ GoogleJsonError.ErrorInfo errorInfo = getErrorInfo(exception);
+ if (errorInfo == null) {
+ serviceCallMetric.call(ServiceCallMetric.CANONICAL_STATUS_UNKNOWN);
+ throw exception;
Review comment:
Adding a throw here changes the existing behaviour. Don't add a throw on this line. The rethrowing is handled below
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/datastore/DatastoreV1.java
##########
@@ -901,12 +907,28 @@ private RunQueryResponse runQueryWithRetries(RunQueryRequest request) throws Exc
Sleeper sleeper = Sleeper.DEFAULT;
BackOff backoff = RUNQUERY_BACKOFF.backoff();
while (true) {
+ HashMap<String, String> baseLabels = new HashMap<>();
+ baseLabels.put(MonitoringInfoConstants.Labels.PTRANSFORM, "TODO");
+ baseLabels.put(MonitoringInfoConstants.Labels.SERVICE, "Datastore");
+ baseLabels.put(MonitoringInfoConstants.Labels.METHOD, "runQueryWithRetries");
+ baseLabels.put(MonitoringInfoConstants.Labels.RESOURCE, "TODO");
+ baseLabels.put(
+ MonitoringInfoConstants.Labels.BIGQUERY_PROJECT_ID, request.getProjectId());
+ ServiceCallMetric serviceCallMetric =
+ new ServiceCallMetric(MonitoringInfoConstants.Urns.API_REQUEST_COUNT, baseLabels);
try {
RunQueryResponse response = datastore.runQuery(request);
+ serviceCallMetric.call("ok");
rpcSuccesses.inc();
return response;
} catch (DatastoreException exception) {
rpcErrors.inc();
+ GoogleJsonError.ErrorInfo errorInfo = getErrorInfo(exception);
+ if (errorInfo == null) {
+ serviceCallMetric.call(ServiceCallMetric.CANONICAL_STATUS_UNKNOWN);
+ throw exception;
+ }
+ serviceCallMetric.call(errorInfo.getReason());
Review comment:
Please run pipelines to test and invoke these. Ask the beam dev list for help running a datastore integration test, and tweak it (i.e. try to read from non existant entities)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org