chore: custom exporter for Client Built-in Metrics by surbhigarg92 · Pull Request #3164 · googleapis/java-spanner

surbhigarg92 · 2024-06-19T10:05:26Z

No description provided.

olavloite · 2024-06-24T14:36:10Z

+      throws IOException {
+    MetricServiceSettings.Builder settingsBuilder = MetricServiceSettings.newBuilder();
+    CredentialsProvider credentialsProvider;
+    if (credentials == null) {


Normally, when a user has not set any credentials, we will use the default credentials found in the environment. Is that what will normally happen here as well?
Put another way: Will we normally call SpannerCloudMonitoringExporter.create(..) with the default credentials when the user has not set any specific credentials?

My plan was to use getCredentials when I call SpannerCloudMonitoringExporter.create(..)

Do you mean this method:

java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionOptions.java

Line 1439 in bee79af

public Credentials getCredentials() {

That method is only usable in case the user uses the Connection API (which no one uses directly, meaning it is only usable in combination with the JDBC driver). But you could use the same type of logic and/or move that logic to a place where it is usable directly in the client library.

Would it maybe make more sense to make the Credentials input argument non-nullable, and force anyone who wants to use NoCredentials to supply that explicitly? That would only be for testing purposes, meaning that it would be something that we normally control.

Sorry, not getCredentials .

We will use GoogleCredentials.getApplicationDefault() , similar to how it is used here

Keeping it Nullable for now, this was referred from the current user facing exporter.

olavloite

LGTM, with some minor questions/assumptions.

aabmass · 2024-06-26T16:18:10Z

+    for (AttributeKey<?> key : attributes.asMap().keySet()) {
+      if (SPANNER_PROMOTED_RESOURCE_LABELS.contains(key)) {


It's interesting all the MonitoredResource labels are coming from metric level attributes instead of OTel Resource attributes. None of these could be set in the OTel resource?

@aabmass It can be set, but does it really make a difference if these Monitored resource labels comes from metrics attributes or OTel resource attributes ?

aabmass · 2024-06-26T16:20:01Z

+
+  // These metric labels will be promoted to the spanner monitored resource fields
+  public static final Set<AttributeKey<String>> SPANNER_PROMOTED_RESOURCE_LABELS =
+      ImmutableSet.of(PROJECT_ID_KEY, INSTANCE_ID_KEY, INSTANCE_CONFIG_ID_KEY, LOCATION_ID_KEY);


nit, iirc the project_id MonitoredResource label must always match the project the metrics are being written to, and so it is unnecessary to send it. We've taken this approach in our exporters since there is no benefit in sending it

psx95

Left some thoughts about handling the shutdown mechanism of the exporter.

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

odeke-em

Thanks @surbhigarg92, just some drive by nits/comments.

odeke-em · 2024-08-16T08:36:53Z

+  @Override
+  public CompletableResultCode export(Collection<MetricData> collection) {
+    if (client.isShutdown()) {
+      logger.log(Level.WARNING, "Exporter is shut down");


Nit: "Exporter was already shut down"

Also I think it is an error to try to use an already shutdown exporter, hence perhaps change the log level to Error?

odeke-em · 2024-08-16T08:39:03Z

+            .collect(Collectors.toList());
+
+    // Skips exporting if there's none
+    if (spannerMetricData.isEmpty()) {


Useful to perform an info log so that debugging will reveal truly that .isEmpty() and that data wasn't exported, instead of one scratching their head wondering why metrics weren't exported.

odeke-em · 2024-08-16T08:46:37Z

+
+  private static MetricKind convertMetricKind(MetricData metricData) {
+    switch (metricData.getType()) {
+      case HISTOGRAM:


I'd suggest avoiding the fall through and instead explicitly add common cases on the same line so: case HISTOGRAM, EXPONENTIAL_HISTOGRAM for clarity and to help avoid any future unexpected issues.

odeke-em · 2024-08-16T08:47:19Z

+      case HISTOGRAM:
+      case EXPONENTIAL_HISTOGRAM:
+        return convertHistogramType(metricData.getHistogramData());
+      case LONG_GAUGE:


Ditto about not allowing fall through and instead putting similar case values on the same line.

surbhigarg92 requested review from a team June 19, 2024 10:05

product-auto-label Bot added size: l Pull request size is large. api: spanner Issues related to the googleapis/java-spanner API. labels Jun 19, 2024

surbhigarg92 force-pushed the custom_exporter branch 4 times, most recently from 7f9c298 to a7cbc5f Compare June 19, 2024 11:16

surbhigarg92 changed the title ~~chore: custom exporter~~ chore: custom exporter for Client Built-in Metrics Jun 19, 2024

surbhigarg92 force-pushed the custom_exporter branch from a7cbc5f to 071ccc0 Compare June 19, 2024 11:23

chore: custom exporter

1274a7d

surbhigarg92 force-pushed the custom_exporter branch from 071ccc0 to 1274a7d Compare June 19, 2024 11:24

surbhigarg92 requested review from olavloite and rahul2393 June 19, 2024 16:41

olavloite reviewed Jun 24, 2024

View reviewed changes

aabmass reviewed Jun 24, 2024

View reviewed changes

review comments

4d56672

olavloite approved these changes Jun 26, 2024

View reviewed changes

aabmass approved these changes Jun 26, 2024

View reviewed changes

psx95 reviewed Jun 26, 2024

View reviewed changes

Comment thread google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerCloudMonitoringExporter.java Outdated

psx95 reviewed Jun 26, 2024

View reviewed changes

Modified Shutdown and Flush as per the review comments

67ebc99

surbhigarg92 force-pushed the custom_exporter branch from eff37f6 to 67ebc99 Compare July 3, 2024 10:34

surbhigarg92 added automerge Merge the pull request once unit tests and other checks pass. owlbot:run Add this label to trigger the Owlbot post processor. labels Jul 3, 2024

gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 3, 2024

🦉 Updates from OwlBot post-processor

7631276

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

gcf-merge-on-green Bot merged commit f394dde into googleapis:main Jul 3, 2024

gcf-merge-on-green Bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jul 3, 2024

surbhigarg92 added a commit to surbhigarg92/java-spanner that referenced this pull request Jul 3, 2024

chore: custom exporter for Client Built-in Metrics (googleapis#3164)

30734ef

odeke-em reviewed Aug 16, 2024

View reviewed changes

surbhigarg92 deleted the custom_exporter branch November 1, 2024 10:10

		for (AttributeKey<?> key : attributes.asMap().keySet()) {
		if (SPANNER_PROMOTED_RESOURCE_LABELS.contains(key)) {

Conversation

surbhigarg92 commented Jun 19, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olavloite Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olavloite left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

psx95 left a comment

Choose a reason for hiding this comment

Uh oh!

odeke-em left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

olavloite Jun 26, 2024 •

edited

Loading