-
Notifications
You must be signed in to change notification settings - Fork 72
Feat: client side metrics #1840
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
Conversation
static final String FIRESTORE_LIBRARY_NAME = "firestore_java"; | ||
|
||
// TODO: change to firestore.googleapis.com | ||
public static final String METER_NAME = "custom.googleapis.com/internal/client"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opentelemetry uses dot separation in metrics naming, while monarch uses slash separation. exporting the built-in metrics to other backends might raise naming conflict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could have named the metrics with dot seperation, use view to reformat it with slash separation in the default OTEL instance for monarch. But the metrics from Gax uses slash already, using dot separation inside firestore doesn't solves the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @milaGGL. A few minor comments/nits below.
I'd like us to disable the feature and land this PR
...e-cloud-firestore/src/main/java/com/google/cloud/firestore/telemetry/TelemetryConstants.java
Outdated
Show resolved
Hide resolved
...e-cloud-firestore/src/main/java/com/google/cloud/firestore/telemetry/TelemetryConstants.java
Outdated
Show resolved
Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/telemetry/ClientIdentifier.java
Outdated
Show resolved
Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/telemetry/ClientIdentifier.java
Show resolved
Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreOptions.java
Outdated
Show resolved
Hide resolved
...oud-firestore/src/main/java/com/google/cloud/firestore/telemetry/BuiltinMetricsProvider.java
Outdated
Show resolved
Hide resolved
...e-cloud-firestore/src/main/java/com/google/cloud/firestore/telemetry/EnabledMetricsUtil.java
Outdated
Show resolved
Hide resolved
...e-cloud-firestore/src/main/java/com/google/cloud/firestore/telemetry/EnabledMetricsUtil.java
Show resolved
Hide resolved
...e-cloud-firestore/src/main/java/com/google/cloud/firestore/telemetry/EnabledMetricsUtil.java
Outdated
Show resolved
Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/telemetry/MetricsUtil.java
Outdated
Show resolved
Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/StreamableQuery.java
Show resolved
Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/telemetry/MetricsUtil.java
Show resolved
Hide resolved
</difference> | ||
|
||
<!-- Change the parameter type of internalStream function --> | ||
<difference> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
waiting for #1896 to lift the clirr alert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the decision for #1896 was to use internalApi
rather than making the class package-private. So we'll need to keep this clirr change as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they are changing the internalStream
from protected
to default . So it would affect the public access of that method, which could remove the need to add clirr for the parameter change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Only nit is to add a test to verify the behavior of MonitoredStreamResponseObserver
in face of a retryable or non-retryable error.
BuiltinMetricsProvider
class to collect 4 GAX layer and 4 SDK layer metricsGoogleCloudMetricExporter
to export metrics data to CloudMonitoringcustom.googleapis.com
namespace (Waiting for BE setup)BEGIN_COMMIT_OVERRIDE
refactor: client side metrics implementation
END_COMMIT_OVERRIDE