xh123637 opened a new pull request, #41021:
URL: https://github.com/apache/beam/pull/41021
Fixes #27038
## What happened
A ParDo invokes `Setup` with a context derived from the bundle context,
while the bundle methods use a different derived context. Both carry the
same PTransform ID, but each context got its own counterset in the
bundle's metrics store, so increments to the same metric diverged into
different metric objects and only the first-registered object was ever
reported. Concretely, a counter incremented once in `Setup` and three
times in `ProcessElement` reported 1 instead of 4 -- the bundle-side
increments were silently dropped.
`Combine.Up` had the same family of bug in a different shape: it passed
the raw bundle context to `Setup` without tagging it with the transform
ID, so `CombineFn` Setup metrics were recorded without PTransform
attribution and could not be reported back to the runner.
## Changes
- `sdks/go/pkg/beam/core/metrics/metrics.go`: when a context asks for
its counterset, reuse the counterset already registered in the store
for that PTransform instead of creating a new one. Setup and bundle
contexts for the same transform now share one counterset, so their
increments land in the same metric object. This covers counters,
distributions and gauges, which all resolve through the counterset.
- `sdks/go/pkg/beam/core/runtime/exec/combine.go`: `Combine.Up` now
derives a Setup context with `SetPTransformID`, as `ParDo` does, so
`CombineFn` Setup metrics are attributed to the transform.
## Notes on scope
- The earlier test-only PR #39965 pinned that a context-free `Setup()`
incrementing against `context.Background()` is dropped; that behavior
is unchanged (the framework cannot attribute increments made against
a context it never sees), and its test still passes.
- This change does not alter the case where `Setup` is invoked with a
context that carries no bundle store at all: per the `SetPTransformID`
contract, callers must pass a context returned by `SetBundleID`, which
all in-tree runners do.
## Tests
- New `TestSetupMetricsSharedWithBundle` in
`sdks/go/pkg/beam/core/runtime/exec/setup_metrics_test.go`: the same
counter, distribution and gauge incremented in `Setup` and in
`ProcessElement` (3 elements) report the combined values -- counter 4,
distribution count 4 / sum 70 / min 10 / max 20, gauge 9. Fails before
the fix (counter 1, distribution count 1 / sum 10, gauge 7).
- New `TestCombineSetupMetricsAttribution`: a `CombineFn` Setup counter
is attributed to the Combine's PTransform in the bundle store. Fails
before the fix (transform `""`).
- Existing suites: `pkg/beam/core/metrics` and
`pkg/beam/core/runtime/exec` pass in full; `pkg/beam/core/runtime/harness`
passes; `-race` clean on the metrics package and on the Setup-metrics
tests. `go vet` reports one pre-existing finding in `translate.go:95`
(assignment copies lock value), untouched by this change.
- Not run: the full repo test suite, e2e / portable-runner tests, CI.
## Checklist
- [x] Tests added
- [x] No new dependencies; no license-header changes needed (no new files)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
