Skip to content

fix(bigtable): Use a different t4t7 latencies for directpath#2902

Open
sushanb wants to merge 2 commits intomainfrom
fix_gfe_headers
Open

fix(bigtable): Use a different t4t7 latencies for directpath#2902
sushanb wants to merge 2 commits intomainfrom
fix_gfe_headers

Conversation

@sushanb
Copy link
Copy Markdown
Contributor

@sushanb sushanb commented Apr 23, 2026

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)
  • Rollback plan is reviewed and LGTMed
  • All new data plane features have a completed end to end testing plan

Fixes #<issue_number_goes_here> ☕️

If you write sample code, please follow the samples format.

@sushanb sushanb requested a review from igorbernstein2 April 23, 2026 20:56
@sushanb sushanb requested review from a team as code owners April 23, 2026 20:56
@product-auto-label product-auto-label Bot added size: m Pull request size is medium. api: bigtable Issues related to the googleapis/java-bigtable API. labels Apr 23, 2026
@sushanb sushanb changed the title fix(bigtable): Use a different t4t7 latencies for directpathg fix(bigtable): Use a different t4t7 latencies for directpath Apr 23, 2026
private final AtomicLong totalClientBlockingTime = new AtomicLong(0);

private final AtomicLong grpcMessageSentDelay = new AtomicLong(0);
private final AtomicLong grpcHeadersSentNanos = new AtomicLong(0);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not a volatile Stopwatch? it would communicate the intent better:

  volatile Optional<Stopwatch> faket4t7 = Optional.empty();

public void grpcHeadersSent() {
  faket4t7 = Optional.of(Stopwatch.createStarted());
}

public void grpcHeadersReceived() {
  faket4t7.map(Stopwatch::stop);
}

recordAttemptCompletion() {
  if (sidebandData.getGfeTiming() != null) {
  } else if (faket4t7.isPresent()) {
     ....
  }
}
....

sidebandData.getClusterInfo(),
code,
sidebandData.getGfeTiming());
} else {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is expanding scope to what we talked about. This was supposed to be only for DirectAccess. This implementation expands the scope to any time the header is missing. Which means that we wont be able to tell what we are actually measuring. In our original conversation scoping it only to DA, we could reason that when transport_type is DA, we measuring client headers until response headers. In all other cases we are measuring gfe latency. And if the DistributionCount() drops we know that we couldnt reach a gfe. With this approach, we muddy our ability to reason about the metric

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: bigtable Issues related to the googleapis/java-bigtable API. size: m Pull request size is medium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants