feat: GFE Latency #1473
feat: GFE Latency #1473
Conversation
product-auto-label
bot
added
the
api: spanner
google-cla
bot
added
the
cla: yes
thiagotnunes
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
| import static com.google.cloud.spanner.spi.v1.SpannerRpcViews.*; | ||
|
|
||
| import com.google.cloud.spanner.DatabaseId; | ||
| import io.grpc.*; |
There was a problem hiding this comment.
Please don't use * imports (https://google.github.io/styleguide/javaguide.html#s3.3.1-wildcard-imports)
| ".*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(); |
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
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 = |
There was a problem hiding this comment.
Do we still need this?
| String projectId = database.getInstanceId().getProject(); | ||
| return getTagContext(method, projectId, instanceId, databaseId); | ||
| } catch (IllegalArgumentException e) { | ||
| logger.log(level, e.toString()); |
There was a problem hiding this comment.
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.*; |
There was a problem hiding this comment.
Please don't use * imports
google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GfeLatencyTest.java
Show resolved
Hide resolved
|
@KiranmayiB if you check the integration tests, we are now getting lots of errors like this: Could we either fix those if we think they are necessary, or change the test log configuration to hide those? |
Adding support for collecting GFE Latency and GFE Header missing count metrics