zeroshade commented on code in PR #1655:
URL: https://github.com/apache/iceberg-go/pull/1655#discussion_r3732117056
##########
catalog/rest/rest.go:
##########
@@ -1109,6 +1109,15 @@ func (r *Catalog) tableFromResponse(_ context.Context,
identifier []string, meta
if err != nil {
return nil, fmt.Errorf("failed to initialize metrics reporter:
%w", err)
}
+ // Opt-in: POST scan/commit reports to the catalog's metrics endpoint,
but
+ // only when enabled and the server advertises the endpoint.
+ if config.GetBool(keyReportMetricsEnabled, false) &&
r.endpoints.check(endpointReportMetrics) == nil {
Review Comment:
`config` here is the **merged** table and catalog configuration: the
server's `defaults`, the server's `overrides`, and the properties on the table
response all feed into it. So this condition is satisfied whenever a *server*
sets `rest.metrics-reporting-enabled=true`, and this client will start POSTing
scan and commit telemetry to that server without the user having opted in.
That is the inverse of what the PR description and issue #1236 describe. The
constant's own doc comment says reporting is "disabled by default so existing
users see no new network traffic unless they turn it on" — but "they" here can
be the server. A default that a remote party can flip is not a default.
The comment immediately above this block already articulates the principle:
server-vended reporter selection "should be an explicit decision rather than a
side effect of merge order." Turning on network telemetry is a strictly larger
decision than choosing between reporter implementations, so it should be held
to at least the same standard.
Suggested fix: read enablement only from client-originated configuration —
`r.reporterProps`, as line 1108 already does for reporter selection, or a
dedicated explicit catalog option — and never from the merged `config`. Then
pin the precedence with a test: server `defaults`, server `overrides`, and
table-response properties each setting the key to `true` must leave reporting
off when the client has not enabled it.
##########
catalog/rest/metrics_reporter.go:
##########
@@ -0,0 +1,76 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package rest
+
+import (
+ "context"
+ "log/slog"
+ "net/http"
+ "net/url"
+
+ "github.com/apache/iceberg-go/metrics"
+)
+
+// keyReportMetricsEnabled opts a REST catalog into POSTing scan/commit reports
+// to the catalog's metrics endpoint. It is disabled by default so existing
+// users see no new network traffic unless they turn it on.
+const keyReportMetricsEnabled = "rest.metrics-reporting-enabled"
+
+// restMetricsReporter POSTs metrics reports to a table's REST metrics endpoint
+// (POST .../tables/{table}/metrics). It is bound to a single table's path and
+// satisfies metrics.Reporter.
+type restMetricsReporter struct {
+ baseURI *url.URL
+ cl *http.Client
+ path []string // table metrics path, relative to baseURI
+}
+
+var _ metrics.Reporter = (*restMetricsReporter)(nil)
+
+// Report wraps the report in a ReportMetricsRequest and POSTs it on a
+// background goroutine. Per the Reporter contract it never blocks or fails the
+// observed scan/commit: the send is detached from the caller's cancellation,
+// and any error is logged and swallowed.
+func (rep *restMetricsReporter) Report(ctx context.Context, report
metrics.MetricsReport) {
+ if report == nil {
+ return
+ }
+
+ req := metrics.NewReportMetricsRequest(report)
+ // Detach from the caller's cancellation (the scan/commit is already
done)
+ // while preserving any context values used by the HTTP client.
+ sendCtx := context.WithoutCancel(ctx)
+
+ go func() {
+ defer func() {
+ if r := recover(); r != nil {
+ slog.Default().Warn("iceberg: panic while
reporting metrics to REST catalog", "recovered", r)
+ }
+ }()
+
+ if _, err := doPost[metrics.ReportMetricsRequest, struct{}](
+ sendCtx, rep.baseURI, rep.path, req, rep.cl, nil,
allowNoContent()); err != nil {
+ slog.Default().Warn("iceberg: failed to report metrics
to REST catalog", "error", err)
+ }
+ }()
+}
+
+// Close satisfies [metrics.Reporter]. The reporter is stateless — it borrows
+// the catalog's shared HTTP client rather than owning one — so there is
nothing
+// to release.
+func (rep *restMetricsReporter) Close() error { return nil }
Review Comment:
The comment is accurate that the reporter borrows the catalog's HTTP client
rather than owning one — but as of this PR it does own goroutines, and a no-op
`Close` cannot reclaim them. In-flight reports outlive the catalog with nothing
to cancel them and nothing to wait on, so a caller that closes the catalog and
expects its outbound network activity to stop does not get that.
Suggested fix: once reporting is backed by a catalog-owned worker pool, have
`Close` cancel the pool's context and wait for the workers to return, bounded
by the reporting timeout so shutdown cannot hang either. That makes shutdown
observable and gives the drain path something to test.
##########
catalog/rest/metrics_reporter_test.go:
##########
@@ -0,0 +1,148 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package rest
+
+import (
+ "bytes"
+ "context"
+ "encoding/json"
+ "io"
+ "net/http"
+ "net/url"
+ "testing"
+ "time"
+
+ "github.com/apache/iceberg-go/metrics"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+type capturedRequest struct {
+ method string
+ path string
+ body []byte
+}
+
+// captureTransport records the request it receives and returns 204 No Content,
+// avoiding any real network listener.
+type captureTransport struct {
+ ch chan capturedRequest
+ block <-chan struct{} // if non-nil, RoundTrip waits on it before
responding
+ err error // if non-nil, returned instead of a response
+}
+
+func (c *captureTransport) RoundTrip(r *http.Request) (*http.Response, error) {
+ body, _ := io.ReadAll(r.Body)
+ if c.ch != nil {
+ c.ch <- capturedRequest{method: r.Method, path: r.URL.Path,
body: body}
+ }
+ if c.block != nil {
+ <-c.block
+ }
+ if c.err != nil {
+ return nil, c.err
+ }
+
+ return &http.Response{
+ StatusCode: http.StatusNoContent,
+ Body: io.NopCloser(bytes.NewReader(nil)),
+ Header: make(http.Header),
+ }, nil
+}
+
+func newTestReporter(t *testing.T, tr *captureTransport) *restMetricsReporter {
Review Comment:
Non-blocking. This helper hands the reporter a pre-built relative path, so
the assertion at line 89 only ever checks `/namespaces/db/tables/t/metrics`.
The parts most likely to break in practice are exactly the parts skipped:
`reqPath` composing the full `/v1/{prefix}/...` URL, multi-level namespaces,
and identifier escaping.
Suggested fix: add a test that goes through `tableFromResponse` — or at
minimum `endpointReportMetrics.reqPath` — with a configured prefix and a nested
namespace containing a character that requires escaping, and assert the
complete request path.
##########
catalog/rest/metrics_reporter.go:
##########
@@ -0,0 +1,76 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package rest
+
+import (
+ "context"
+ "log/slog"
+ "net/http"
+ "net/url"
+
+ "github.com/apache/iceberg-go/metrics"
+)
+
+// keyReportMetricsEnabled opts a REST catalog into POSTing scan/commit reports
+// to the catalog's metrics endpoint. It is disabled by default so existing
+// users see no new network traffic unless they turn it on.
+const keyReportMetricsEnabled = "rest.metrics-reporting-enabled"
Review Comment:
The property name here is `rest.metrics-reporting-enabled`, with a dot after
`rest`. Iceberg Java and the Iceberg documentation both use
`rest-metrics-reporting-enabled`, with a hyphen.
That one character makes this feature unreachable for anyone configuring it
the documented, cross-implementation way: they set the canonical key, this code
never sees it, and reporting silently stays off with no error and no warning to
explain why. Silent no-ops on a configuration key are particularly expensive to
debug, since the natural conclusion is that the feature is broken rather than
misnamed.
Suggested fix: make `rest-metrics-reporting-enabled` the canonical key. If
the dotted spelling is worth keeping, accept it as a deliberate alias rather
than as the only accepted name. It would also help to export the constant, or
expose a typed catalog option, so callers are not hardcoding the string at all.
##########
catalog/rest/metrics_reporter.go:
##########
@@ -0,0 +1,76 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package rest
+
+import (
+ "context"
+ "log/slog"
+ "net/http"
+ "net/url"
+
+ "github.com/apache/iceberg-go/metrics"
+)
+
+// keyReportMetricsEnabled opts a REST catalog into POSTing scan/commit reports
+// to the catalog's metrics endpoint. It is disabled by default so existing
+// users see no new network traffic unless they turn it on.
+const keyReportMetricsEnabled = "rest.metrics-reporting-enabled"
+
+// restMetricsReporter POSTs metrics reports to a table's REST metrics endpoint
+// (POST .../tables/{table}/metrics). It is bound to a single table's path and
+// satisfies metrics.Reporter.
+type restMetricsReporter struct {
+ baseURI *url.URL
+ cl *http.Client
+ path []string // table metrics path, relative to baseURI
+}
+
+var _ metrics.Reporter = (*restMetricsReporter)(nil)
+
+// Report wraps the report in a ReportMetricsRequest and POSTs it on a
+// background goroutine. Per the Reporter contract it never blocks or fails the
+// observed scan/commit: the send is detached from the caller's cancellation,
+// and any error is logged and swallowed.
+func (rep *restMetricsReporter) Report(ctx context.Context, report
metrics.MetricsReport) {
+ if report == nil {
+ return
+ }
+
+ req := metrics.NewReportMetricsRequest(report)
+ // Detach from the caller's cancellation (the scan/commit is already
done)
+ // while preserving any context values used by the HTTP client.
+ sendCtx := context.WithoutCancel(ctx)
+
+ go func() {
Review Comment:
Every call to `Report` spawns a goroutine that nothing tracks and nothing
bounds. Paired with the missing timeout above, a metrics endpoint that accepts
connections but never responds accumulates one permanently blocked goroutine —
and one held connection — per scan and per commit, for as long as the process
runs.
That is where the isolation guarantee stops holding. Any individual report
is harmless, but the unbounded accumulation can exhaust connections and
eventually degrade the very scans and commits this reporter is meant to stay
out of the way of.
Suggested fix: move dispatch to a catalog-owned bounded queue served by a
small fixed worker pool, rather than spawning per report. When the queue is
full, drop the report and log the drop — shedding telemetry is the correct
behavior here, and logging it makes back-pressure visible instead of silent.
Tracking the workers also gives `Close` something to drain (see below).
##########
catalog/rest/metrics_reporter.go:
##########
@@ -0,0 +1,76 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package rest
+
+import (
+ "context"
+ "log/slog"
+ "net/http"
+ "net/url"
+
+ "github.com/apache/iceberg-go/metrics"
+)
+
+// keyReportMetricsEnabled opts a REST catalog into POSTing scan/commit reports
+// to the catalog's metrics endpoint. It is disabled by default so existing
+// users see no new network traffic unless they turn it on.
+const keyReportMetricsEnabled = "rest.metrics-reporting-enabled"
+
+// restMetricsReporter POSTs metrics reports to a table's REST metrics endpoint
+// (POST .../tables/{table}/metrics). It is bound to a single table's path and
+// satisfies metrics.Reporter.
+type restMetricsReporter struct {
+ baseURI *url.URL
+ cl *http.Client
+ path []string // table metrics path, relative to baseURI
+}
+
+var _ metrics.Reporter = (*restMetricsReporter)(nil)
+
+// Report wraps the report in a ReportMetricsRequest and POSTs it on a
+// background goroutine. Per the Reporter contract it never blocks or fails the
+// observed scan/commit: the send is detached from the caller's cancellation,
+// and any error is logged and swallowed.
+func (rep *restMetricsReporter) Report(ctx context.Context, report
metrics.MetricsReport) {
+ if report == nil {
+ return
+ }
+
+ req := metrics.NewReportMetricsRequest(report)
+ // Detach from the caller's cancellation (the scan/commit is already
done)
+ // while preserving any context values used by the HTTP client.
+ sendCtx := context.WithoutCancel(ctx)
Review Comment:
Detaching from the caller is the right instinct — the scan or commit has
already finished and its cancellation should not abort the report. The problem
is that `context.WithoutCancel` removes the caller's deadline without
installing one of its own, and the catalog's shared `http.Client` has no
`Timeout` set either. This request therefore has no bound at any layer.
Concretely, that means an unreachable or black-holing endpoint, a token
refresh that never returns, a wait on response headers, or a response body that
never finishes draining will block for the lifetime of the process.
Suggested fix: keep the detach, and add a deadline of its own:
```go
sendCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx),
reportTimeout)
```
with `defer cancel()` inside the goroutine so it covers auth plus the
complete request and response cycle. A conservative default of a few seconds,
ideally configurable, seems right for telemetry that is safe to drop.
##########
catalog/rest/metrics_reporter_test.go:
##########
@@ -0,0 +1,148 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package rest
+
+import (
+ "bytes"
+ "context"
+ "encoding/json"
+ "io"
+ "net/http"
+ "net/url"
+ "testing"
+ "time"
+
+ "github.com/apache/iceberg-go/metrics"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+type capturedRequest struct {
Review Comment:
Non-blocking. `capturedRequest` records only the method, path, and body, so
no test can currently assert anything about headers. Reusing the catalog's
authenticated client is one of the more valuable properties of this design, and
right now nothing verifies it.
Suggested fix: capture `r.Header` as well, then assert that the metrics POST
carries the `Authorization` header — including after a token refresh — along
with `Content-Type: application/json` and any headers configured on the catalog.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
