laskoviymishka commented on code in PR #1343: URL: https://github.com/apache/iceberg-go/pull/1343#discussion_r3518187069
########## metrics/reporters.go: ########## @@ -0,0 +1,157 @@ +// 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 metrics + +import ( + "context" + "fmt" + "log/slog" + "sync" +) + +// NopReporter is a [Reporter] that discards every report. It is the default +// when no reporter is configured, so that instrumentation is free unless a user +// opts in. The zero value is ready to use. +type NopReporter struct{} + +var _ Reporter = NopReporter{} + +// Report implements [Reporter] and does nothing. +func (NopReporter) Report(context.Context, MetricsReport) {} + +// LoggingReporter is a [Reporter] that logs each report via an [slog.Logger]. It +// is a convenient default for development and debugging. +type LoggingReporter struct { + logger *slog.Logger // nil means resolve slog.Default at call time +} + +var _ Reporter = (*LoggingReporter)(nil) + +// NewLoggingReporter returns a [LoggingReporter] that logs to logger. If logger +// is nil, [slog.Default] is resolved at each Report call, so a later +// [slog.SetDefault] is honored rather than snapshotted at construction. +func NewLoggingReporter(logger *slog.Logger) *LoggingReporter { + return &LoggingReporter{logger: logger} +} + +// Report logs report at info level. +func (r *LoggingReporter) Report(ctx context.Context, report MetricsReport) { + if report == nil { + return + } + logger := r.logger + if logger == nil { + logger = slog.Default() + } + logger.InfoContext(ctx, "iceberg metrics report", "report", report) +} + +// InMemoryReporter is a [Reporter] that retains every report it receives. It is +// primarily intended for tests and inspection. It is safe for concurrent use. +type InMemoryReporter struct { + mu sync.Mutex + reports []MetricsReport +} + +var _ Reporter = (*InMemoryReporter)(nil) + +// Report appends report to the retained set. +func (r *InMemoryReporter) Report(_ context.Context, report MetricsReport) { + if report == nil { + return + } + r.mu.Lock() + defer r.mu.Unlock() + r.reports = append(r.reports, report) +} + +// Reports returns a copy of the reports received so far, in arrival order. +func (r *InMemoryReporter) Reports() []MetricsReport { + r.mu.Lock() + defer r.mu.Unlock() + + return append([]MetricsReport(nil), r.reports...) +} + +// Reset discards all retained reports. +func (r *InMemoryReporter) Reset() { + r.mu.Lock() + defer r.mu.Unlock() + r.reports = nil +} + +// Combine returns a [Reporter] that forwards each report to all of the given +// reporters in order. nil reporters are skipped. A panic in one reporter must +// not prevent the others from receiving the report, so each call is isolated; +// in keeping with the [Reporter] contract a misbehaving reporter never affects +// the observed operation. A recovered panic is logged (with the reporter type) +// at warn level via [slog.Default] so a broken reporter is not silently +// swallowed. +// +// As a convenience, Combine with no non-nil reporters returns [NopReporter]. +// Otherwise every reporter — even a lone one — is wrapped so it receives the +// per-reporter panic isolation the contract advertises. Wrapping a single +// reporter also closes a typed-nil hole: a concrete nil pointer (e.g. +// (*LoggingReporter)(nil)) is a non-nil interface, so it passes the nil filter; +// returning it unwrapped would let its eventual Report nil-deref escape with no +// recover to catch it. +func Combine(reporters ...Reporter) Reporter { + nonNil := make([]Reporter, 0, len(reporters)) + for _, r := range reporters { + if r != nil { + nonNil = append(nonNil, r) + } + } + + if len(nonNil) == 0 { + return NopReporter{} + } + + return &compositeReporter{reporters: nonNil} +} + +// compositeReporter fans a report out to several reporters, isolating each from +// the others' panics. +type compositeReporter struct { + reporters []Reporter + logger *slog.Logger // nil means resolve slog.Default at call time Review Comment: This field is never set — `Combine` is the only constructor and it builds `&compositeReporter{reporters: nonNil}`, so `logger` is permanently nil and the nil-fallback in the recover only works because of that. It reads like configurable state that isn't. I'd either drop the field and call `slog.Default()` directly in the recover, or add a `CombineWithLogger(logger, reporters...)` that actually wires it. Dropping it is simpler — unless you want the injectable logger for the panic-log test below, in which case they're worth deciding together. ########## metrics/reporter.go: ########## @@ -0,0 +1,64 @@ +// 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 metrics implements Iceberg's Metrics Reporting API for iceberg-go. +// +// A [Reporter] is a pluggable sink that receives a [MetricsReport] — a +// ScanReport after scan planning or a CommitReport after a commit — describing +// what the client did (files and manifests considered, scanned and skipped; +// bytes read; commit attempts and durations). Reporting gives operators a +// standard way to aggregate these otherwise-invisible client-side metrics +// across many clients. +// +// Reporting is strictly opt-in: with no reporter configured the instrumented +// code paths do no work. Reporters must never block or fail the scan/commit +// they observe — see [Reporter] for the contract. +// +// This package provides the contract and the built-in reporters +// ([NopReporter], [LoggingReporter], [InMemoryReporter], and [Combine]). The +// concrete report types and the scan/commit instrumentation are layered on top +// in later work. +package metrics + +import "context" + +// MetricsReport is the marker interface implemented by the concrete report +// types (ScanReport and CommitReport). It is intentionally empty, mirroring the +// open MetricsReport interface in the Java and Python Iceberg implementations, +// so downstream operators can implement it for their own report wrappers. +// +// The set of report types is controlled by this package for now, but that is a +// convention documented here rather than a structural guarantee — the interface +// is deliberately not sealed, leaving room for third-party reports without a +// future breaking change. +// +// The name mirrors the MetricsReport type in the Java and Python Iceberg +// implementations, easing ports across the ecosystem and disambiguating the +// type from the [Reporter.Report] method that consumes it. +type MetricsReport any Review Comment: I'd make this a distinct named interface rather than an alias for `any` — `type MetricsReport interface{}`. Right now `reporter.Report(ctx, 42)` compiles, and when the REST reporter lands it'll type-switch over an unrestricted interface with nothing to verify arm coverage. Java's is a real (empty) interface enforced by `instanceof`. Zero cost to tighten now, and a more disruptive break once concrete types ship downstream. wdyt? ########## metrics/reporters_test.go: ########## @@ -0,0 +1,188 @@ +// 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 metrics + +import ( + "bytes" + "context" + "log/slog" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// testReport is a minimal MetricsReport used to exercise the reporters until +// the concrete report types (ScanReport, CommitReport) land. MetricsReport is +// an open marker interface, so any type satisfies it. +type testReport struct{ name string } + +// countingReporter records how many reports it received. +type countingReporter struct { + mu sync.Mutex + count int +} + +func (c *countingReporter) Report(context.Context, MetricsReport) { + c.mu.Lock() + defer c.mu.Unlock() + c.count++ +} + +func (c *countingReporter) calls() int { + c.mu.Lock() + defer c.mu.Unlock() + + return c.count +} + +// panickingReporter always panics, to verify Combine isolates failures. +type panickingReporter struct{} + +func (panickingReporter) Report(context.Context, MetricsReport) { panic("boom") } + +func TestNopReporter(t *testing.T) { + // NopReporter must accept anything, including nil, without panicking. + assert.NotPanics(t, func() { + NopReporter{}.Report(context.Background(), nil) + NopReporter{}.Report(context.Background(), testReport{}) + }) +} + +func TestInMemoryReporter(t *testing.T) { + var r InMemoryReporter + require.Empty(t, r.Reports()) + + r.Report(context.Background(), nil) // ignored + require.Empty(t, r.Reports()) + + r.Report(context.Background(), testReport{name: "a"}) + r.Report(context.Background(), testReport{name: "b"}) + + got := r.Reports() + require.Len(t, got, 2) + assert.Equal(t, testReport{name: "a"}, got[0]) + assert.Equal(t, testReport{name: "b"}, got[1]) + + // Reports returns a copy: mutating it must not affect the reporter. + got[0] = nil + require.Len(t, r.Reports(), 2) + + r.Reset() + require.Empty(t, r.Reports()) +} + +func TestLoggingReporterNilLoggerAndReport(t *testing.T) { + t.Run("nil logger falls back to default", func(t *testing.T) { + r := NewLoggingReporter(nil) // must fall back to slog.Default + require.NotNil(t, r) + assert.NotPanics(t, func() { + r.Report(context.Background(), nil) + r.Report(context.Background(), testReport{name: "x"}) + }) + }) + + t.Run("logs the report at info level", func(t *testing.T) { + var buf bytes.Buffer + r := NewLoggingReporter(slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelInfo}))) + + // A nil report is ignored and must produce no output. + r.Report(context.Background(), nil) + require.Empty(t, buf.String()) + + r.Report(context.Background(), testReport{name: "scan-42"}) + out := buf.String() + assert.Contains(t, out, "level=INFO") + assert.Contains(t, out, "iceberg metrics report") + assert.Contains(t, out, "scan-42", "the report value must be logged") + }) +} + +func TestCombine(t *testing.T) { + t.Run("no reporters returns Nop", func(t *testing.T) { + assert.IsType(t, NopReporter{}, Combine()) + assert.IsType(t, NopReporter{}, Combine(nil, nil)) + }) + + t.Run("single reporter is wrapped for isolation", func(t *testing.T) { + // Even one reporter is wrapped, so a typed-nil concrete pointer (a + // non-nil interface that passes the nil filter) still gets the panic + // isolation the contract promises instead of escaping unwrapped. + var lr *LoggingReporter // (*LoggingReporter)(nil) as a Reporter is non-nil + r := Combine(lr) + assert.IsType(t, &compositeReporter{}, r) + assert.NotPanics(t, func() { + r.Report(context.Background(), testReport{name: "x"}) + }) + }) + + t.Run("fans out to all", func(t *testing.T) { + a, b := &countingReporter{}, &countingReporter{} + Combine(a, b).Report(context.Background(), testReport{}) + assert.Equal(t, 1, a.calls()) + assert.Equal(t, 1, b.calls()) + }) + + t.Run("isolates a panicking reporter", func(t *testing.T) { Review Comment: This proves the panic is isolated and the next reporter runs, but it doesn't cover the warn-log path — nothing asserts `WarnContext` fired, or that the output carries the reporter type and the panic value. If a future change drops the `WarnContext` call, this stays green. Same trick as the LoggingReporter test would do it: build the composite with a buffer-backed logger and assert the output contains "panicked", the type name, and "boom". That's currently blocked by the un-injectable logger field above, so worth deciding the two together. -- 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]
