Skip to content
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

feat: GFE Latency #1473

Merged
merged 9 commits into from Oct 7, 2021
Merged

feat: GFE Latency #1473

merged 9 commits into from Oct 7, 2021

Conversation

KiranmayiB
Copy link
Contributor

@KiranmayiB KiranmayiB commented Sep 29, 2021

Adding support for collecting GFE Latency and GFE Header missing count metrics

@KiranmayiB KiranmayiB requested review from as code owners Sep 29, 2021
@product-auto-label product-auto-label bot added the api: spanner label Sep 29, 2021
@google-cla google-cla bot added the cla: yes label Sep 29, 2021
@thiagotnunes thiagotnunes added the kokoro:force-run label Sep 30, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Sep 30, 2021
import static com.google.cloud.spanner.spi.v1.SpannerRpcViews.*;

import com.google.cloud.spanner.DatabaseId;
import io.grpc.*;
Copy link
Contributor

@thiagotnunes thiagotnunes Oct 5, 2021

Choose a reason for hiding this comment

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

".*projects/(?<project>\\w\\p{ASCII}+)/instances/(?<instance>\\w\\p{ASCII}+)/databases/(?<database>\\w\\p{ASCII}+)");

// Get the global singleton Tagger object.
private static final Tagger tagger = Tags.getTagger();
Copy link
Contributor

@thiagotnunes thiagotnunes Oct 5, 2021

Choose a reason for hiding this comment

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

Please use snake upper case for constants (static final): https://google.github.io/styleguide/javaguide.html#s5.2.4-constant-names

measureMap.put(SPANNER_GFE_LATENCY, latency).record(tagContext);
measureMap.put(SPANNER_GFE_HEADER_MISSING_COUNT, 0L).record(tagContext);
} catch (NumberFormatException e) {
logger.log(level, "invalid server-timing object in header");
Copy link
Contributor

@thiagotnunes thiagotnunes Oct 5, 2021

Choose a reason for hiding this comment

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

Maybe add the matcher.group("dur") in the log message as well

private static final Pattern SERVER_TIMING_HEADER_PATTERN = Pattern.compile(".*dur=(?<dur>\\d+)");
private static final Metadata.Key<String> GOOGLE_CLOUD_RESOURCE_PREFIX_KEY =
Metadata.Key.of("google-cloud-resource-prefix", Metadata.ASCII_STRING_MARSHALLER);
private static final Pattern GOOGLE_CLOUD_RESOURCE_PREFIX_PATTERN =
Copy link
Contributor

@thiagotnunes thiagotnunes Oct 5, 2021

Choose a reason for hiding this comment

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

Do we still need this?

String projectId = database.getInstanceId().getProject();
return getTagContext(method, projectId, instanceId, databaseId);
} catch (IllegalArgumentException e) {
logger.log(level, e.toString());
Copy link
Contributor

@thiagotnunes thiagotnunes Oct 5, 2021

Choose a reason for hiding this comment

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

Could you log the whole exception with a message as well?

Maybe logger.log(level, "Error parsing google cloud resource header", e)


import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
import io.opencensus.stats.*;
Copy link
Contributor

@thiagotnunes thiagotnunes Oct 5, 2021

Choose a reason for hiding this comment

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

Please don't use * imports

@thiagotnunes
Copy link
Contributor

@thiagotnunes thiagotnunes commented Oct 5, 2021

@KiranmayiB if you check the integration tests, we are now getting lots of errors like this:

INFO: java.lang.IllegalArgumentException: Name should conform to pattern projects/{project=*}/instances/{instance=*}/databases/{database=*}: projects/gcloud-devel/instances/spanner-testing

Could we either fix those if we think they are necessary, or change the test log configuration to hide those?

@thiagotnunes thiagotnunes merged commit de82f78 into googleapis:main Oct 7, 2021
17 checks passed
@KiranmayiB KiranmayiB deleted the gfe_latency branch Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants