ajamato commented on a change in pull request #13491:
URL: https://github.com/apache/beam/pull/13491#discussion_r555442985
##########
File path:
sdks/java/extensions/google-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/LatencyRecordingHttpRequestInitializer.java
##########
@@ -23,14 +23,26 @@
import com.google.api.client.http.HttpResponse;
import com.google.api.client.http.HttpResponseInterceptor;
import java.io.IOException;
-import org.apache.beam.sdk.util.Histogram;
+import org.apache.beam.sdk.metrics.Histogram;
+import org.apache.beam.sdk.metrics.Metrics;
+import org.apache.beam.sdk.util.HistogramData;
/** HttpRequestInitializer for recording request to response latency of
Http-based API calls. */
public class LatencyRecordingHttpRequestInitializer implements
HttpRequestInitializer {
+ public static final String HISTOGRAM_NAME = "latency_ms";
Review comment:
Would you please use a specific name for the metric where it is
instantiated
See the URN defined here:
API_REQUEST_LATENCIES
urn = beam:metric:io:api_request_latencies:v1
https://s.apache.org/beam-gcp-debuggability
For Labels:
for SERVICE_NAME use "BigQuery", METHOD_NAME you can use
"BigQueryBatchWrite" and "BigQueryBatchRead". Which is just referring to API
calls which read or write elements in batches.
Additional Labels (Populate these if they are available, if not leave it for
follow up work for me):
BIGQUERY_PROJECT_ID
BIGQUERY_DATASET
BIGQUERY_TABLE
BIGQUERY_VIEW
BIGQUERY_QUERY_NAME- user provided query name
RESOURCE - combination of the above (See my python PR linked below for
reference)
https://github.com/apache/beam/pull/13217/files#diff-5427a5d3887eb695cefde082c58575a2372972996b547d55961abeb4f7bc3debR576
##########
File path:
sdks/java/core/src/main/java/org/apache/beam/sdk/metrics/DelegatingCounter.java
##########
@@ -38,7 +44,10 @@ public void inc() {
/** Increment the counter by the given amount. */
@Override
public void inc(long n) {
- MetricsContainer container = MetricsEnvironment.getCurrentContainer();
+ MetricsContainer container =
+ processWideContainer
Review comment:
this.processWideContainer
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/metrics/Metrics.java
##########
@@ -96,6 +97,21 @@ public static Gauge gauge(Class<?> namespace, String name) {
return new DelegatingGauge(MetricName.named(namespace, name));
}
+ public static class Internal {
+ public static Counter counter(String namespace, String name, boolean
processWideContainer) {
+ return new DelegatingCounter(MetricName.named(namespace, name),
processWideContainer);
+ }
+
+ public static Histogram histogram(
+ String namespace,
Review comment:
namespace and name parameters are specific to user metric (Which is a
metric that populates a specific MonitoringInfo URN with a name and namspace
parameter).
Have this just take a MetricName instead which allows for non user metrics
I.e. like the ElementCount metric
which is uniquely identifiers by a URN and labels.
https://github.com/apache/beam/pull/7272/files#diff-3834ef03b753005ee8251870a88fe3505732e9a7ad34da600f78e33ab9f37139R45
You can make it under the LabeledMetrics class, which is meant to be an
internal implementation for SDK harness implementations. Not part of the public
API (Much like the internal.Metrics in python you've added).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
