[shunping]

Summary

Today --gcsPerformanceMetrics only emits four counters, covering copy and rename. That is not enough to capture the possible performance change between package versions.

In this PR, we makes the following improvements:

• Move every GCS counter into a single Gcs namespace, with the layer encoded in the metric name (gcs_op_* for filesystem operations, gcs_http_* for transport).
• Extend operation-level coverage from 2 operations to 7, each with a _count/_msec pair.
  Adds transport-level counters (request counts by shape, response counts by HTTP status class, and wire bytes) by attaching interceptors to the GCS HttpRequestInitializer, bound to the MetricsContainer of the step that opened the channel.
• Everything remains behind the existing --gcsPerformanceMetrics flag, which defaults to false, so the default behavior of a pipeline is unchanged apart from the metric renames.

Metric changes

Operation level (gcs_op_*, namespace Gcs)

Metric pair	Counts	Notes
gcs_op_copy_count / _msec	objects copied	renamed from num_copies / copy_time_msec
gcs_op_rename_count / _msec	objects renamed	renamed from num_renames / rename_time_msec
gcs_op_delete_count / _msec	objects deleted	new
gcs_op_match_glob_count / _msec	glob expansions	new
gcs_op_match_nonglob_count / _msec	paths looked up	new, empty batches are skipped so they do not dilute the average
gcs_op_open_count / _msec	channels opened	new, time covers channel setup only, not the transfer
gcs_op_create_count / _msec	channels created	new, time covers channel setup only, not the transfer
gcs_op_match_glob_pages	object listing pages fetched while expanding globs	new, standalone counter

Transport level (gcs_http_*, namespace Gcs)

Metric	Read	Write	Meaning
gcs_http_*_request_count	✅	✅	every attempt, retries included
gcs_http_*_request_count_ranged	✅	—	GET carrying a Range header
gcs_http_*_request_count_unbounded	✅	—	GET without a Range header (includes metadata GETs)
gcs_http_*_request_count_other	✅	—	non-GET, e.g. the POST of a batched metadata lookup
gcs_http_*_status_2xx	✅	✅	success, includes 206 Partial Content
gcs_http_*_status_3xx	✅	✅	not an error: a resumable upload answers 308 to every chunk but the last
gcs_http_*_status_4xx	✅	✅	client-side error
gcs_http_*_status_5xx	✅	✅	server-side error
gcs_http_*_status_other	✅	✅	1xx
gcs_http_*_request_no_response	✅	✅	attempt failed before a response arrived, and was retried or propagated
gcs_http_read_wire_bytes_received	✅	—	bytes read off the channel
gcs_http_write_wire_bytes_sent	—	✅	bytes written to the channel

[shunping]

r: @Abacn

[github-actions]

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment assign set of reviewers

[shunping]

Right now, we only trigger the minimum configuration ("local") in our github postcommit workflow. There will be a separate PR to add a new workflow to sweep all the configurations weekly.

[Abacn]

There are workflows running large tests:

https://github.com/apache/beam/blob/master/.github/workflows/beam_StressTests_Java_BigQueryIO.yml

https://github.com/apache/beam/blob/master/.github/workflows/beam_StressTests_Java_KafkaIO.yml

and others.

Large test should have their own workflows as it runs for long

[shunping]

Agreed! I will have a follow-up PR to set up those tests.

[Abacn]

Just curious, is this an optimization and do we know how much performance gain from it?

[shunping]

Yes, it is an optimization.

Previously we retrieve the row group information for every getSize() and newTracker() on every dynamic split attempt or progress probing. In one of the tests, this fix reduced the number of metadata queries by ~80%.

[Abacn]

There are workflows running large tests:

https://github.com/apache/beam/blob/master/.github/workflows/beam_StressTests_Java_BigQueryIO.yml

https://github.com/apache/beam/blob/master/.github/workflows/beam_StressTests_Java_KafkaIO.yml

and others.

Large test should have their own workflows as it runs for long

[Abacn]

It's testing code and we don't need complicated parsers (regex) and even handling alias switch (unit) { ... }. Can we just use bytes or simple suffix?

[shunping]

Reasonable. Already simplified accordingly.

[Abacn]

getGcsMetrics and printGcsMetrics nearly identical but long codes. Consider consolidate them.

[shunping]

Refactored metric handling and reporting in MetricReport class to keep GcsIOLoadTestBase light-weight and GCS-oriented. We also make such metric handling code reusable.

[Abacn]

This duplicates the same helper in GcsIOLoadTestBase

[shunping]

Moved it to a new utility class.

[Abacn]

If previous comments are addressed I'd expect the Pr would have a much smaller footprint. Yet it keep growing into larger changes. For example now we have a whole "ByteSizeUtil" and need to add test to it. This is opposite to my comments about simplify things

[shunping]

The parsing logic is simplified as we don't have to use regex to support different annotations. And to facilitate future usage, I think extracting the functions to a utility class will improve its reusability. Even though we have more code, it is covered by necessary tests.

[Abacn]

This is just an example one specific point gets further complicated rather than simplified after iteration

For this specific point, a few lines of code is sufficient like

private static final ImmutableMap<String, Long> UNIT_MAP =
      ImmutableMap.of(
          "k", 1024L, "m", 1024L * 1024, "g", 1024L * 1024 * 1024, "t", 1024L * 1024 * 1024 * 1024);

  private static long getBytesFromOption(String raw) {
    int len = raw.length();
    if (len >= 2 && UNIT_MAP.containsKey(raw.substring(len - 1).toLowerCase())) {
      return Long.valueOf(raw.substring(0, len - 1))
          * UNIT_MAP.get(raw.substring(len - 1).toLowerCase());
    }
    return Long.valueOf(raw);
  }

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.25%. Comparing base (2448f5a) to head (fb7c3c9).

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #40142      +/-   ##
============================================
- Coverage     59.59%   57.25%   -2.35%     
+ Complexity    17598     3695   -13903     
============================================
  Files          3050     1201    -1849     
  Lines        293086   194280   -98806     
  Branches      13816     3890    -9926     
============================================
- Hits         174679   111238   -63441     
+ Misses       111287    79437   -31850     
+ Partials       7120     3605    -3515     

Flag	Coverage Δ
java	72.07% <ø> (+5.95%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[shunping]

Since this PR is getting bigger, let me split it into two or three for easier review.

[Abacn]

If agent is used, it shows strong tendency of

• unrelated change gets touched whenever agent thought it can "improve" things when loitering code paths

• over-engineering trivial logics

[shunping]

I trimmed the original and only keep the changes of GCS metrics here. PTAL. @Abacn

These new metrics are pre-requisites to the follow-up GCS load tests.

[Abacn]

The Javadoc states that status_2xx + status_3xx + status_4xx + status_5xx + status_other + request_no_response == request_count. This isn't true whenever there is retry inside HttpResponseInterceptor:

However, in google-http-client's HttpRequest.java.execute(), HttpExecuteInterceptor.intercept() is called inside the do { ... } while (retryRequest) loop (once per attempt), whereas HttpResponseInterceptor.interceptResponse() is called outside/after the retry loop (only once for the final response).

We can simply drop inaccurate javadocs

[shunping]

Fixed the javadocs.

[Abacn]

This creates an unclosed GoogleCloudStorageImpl instance on every GcsUtilV1.open() when metrics are enabled, may have performance implications

One of the original suspected reason for OOM was gRPC cache size grew. It wasn't the case, but with this change it could surface.

[shunping]

Done. Cached the storage instance by metric container and the resource will be removed when metric container is gc'ed.

[Abacn]

I'm not sure if we need cache here. metrics container isn't owned by either GcsUtil or Impl. In fact, it's a runtime static object

container = MetricsEnvironment.getCurrentContainer();

Can we simply pass a flag down to Transport, then it get metrics container in place? I suspect it's passing container is to acommodate unit tests, which isn't worth it at the cost of holding additional state or excessive client creations.

[shunping]

MetricsEnvironment.getCurrentContainer() is backed by a ThreadLocal, which the runner only sets on the DoFn thread for the active step.

Calling MetricsEnvironment.getCurrentContainer() inside Transport when HTTP requests execute doesn't work because:

• gcsio executes HTTP requests on background thread pools, where Beam's ThreadLocal is not propagated and getCurrentContainer() returns null. By capturing container in open()/create() on the DoFn thread, we can make sure background threads can increment the counters directly without relying on the ThreadLocal and avoid metric loss.
• GoogleCloudStorageImpl fixes its HttpRequestInitializer at construction time. Since each step has its own MetricsContainer, caching GoogleCloudStorage per MetricsContainer ensures we create at most one client per step rather than one per file.