Skip to content

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

Merged
merged 46 commits into from
Nov 5, 2024
Merged

Feat: client side metrics #1840

merged 46 commits into from
Nov 5, 2024

Conversation

milaGGL
Copy link
Contributor

@milaGGL milaGGL commented Sep 19, 2024

  • Integrate Opentelemetry API and SDK into the Firestore SDK
  • Implement BuiltinMetricsProvider class to collect 4 GAX layer and 4 SDK layer metrics
  • Integrate GoogleCloudMetricExporter to export metrics data to CloudMonitoring custom.googleapis.com namespace (Waiting for BE setup)
  • Create public API to enable/disable client side metrics (the API is set to private, and disaled for now.)

BEGIN_COMMIT_OVERRIDE
refactor: client side metrics implementation
END_COMMIT_OVERRIDE

@product-auto-label product-auto-label bot added size: l Pull request size is large. api: firestore Issues related to the googleapis/java-firestore API. labels Sep 19, 2024
@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Sep 20, 2024
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";
Copy link
Contributor Author

@milaGGL milaGGL Sep 27, 2024

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@ehsannas ehsannas left a 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

@milaGGL milaGGL assigned ehsannas and unassigned milaGGL Oct 21, 2024
@milaGGL milaGGL requested a review from ehsannas October 21, 2024 15:20
</difference>

<!-- Change the parameter type of internalStream function -->
<difference>
Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

@milaGGL milaGGL Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they are changing the internalStreamfrom 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.

Copy link
Contributor

@ehsannas ehsannas left a 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.

@milaGGL milaGGL assigned milaGGL and unassigned ehsannas Nov 4, 2024
@milaGGL milaGGL merged commit 77763c0 into main Nov 5, 2024
23 of 24 checks passed
@milaGGL milaGGL deleted the mila/client-side-metrics-demo2 branch November 5, 2024 17:49
@MarkDuckworth MarkDuckworth added the release-please:force-run To run release-please label Nov 5, 2024
@release-please release-please bot removed the release-please:force-run To run release-please label Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/java-firestore API. size: xl Pull request size is extra large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants