laskoviymishka commented on code in PR #1343: URL: https://github.com/apache/iceberg-go/pull/1343#discussion_r3539677725
########## 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 Review Comment: The "without a future breaking change" framing reads backwards — keeping the marker open is the irreversible call; a future attempt to *seal* it is what would break. And the extensibility it advertises won't reach the REST path: once the REST reporter lands it can only serialize the discriminators it knows (`scan-report`/`commit-report`), so a third-party `MetricsReport` has no discriminator and a spec-compliant server rejects it. I'd soften this to say the marker is open for the local reporters but that custom report types are silently dropped by the REST reporter — open extensibility for `Logging`/`InMemory`, not for REST. Not relitigating the open decision (still think it's right); I just don't want the doc to over-promise. wdyt? ########## 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 Review Comment: The overview lists `Combine` but not `CombineWithLogger`, which landed in the same round — worth adding it here so the package doc matches the exported surface. ########## metrics/reporters.go: ########## @@ -0,0 +1,166 @@ +// 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 { Review Comment: One concrete edge the open `any` introduces: this only catches an untyped nil. Once `ScanReport`/`CommitReport` arrive as pointer types, a `(*ScanReport)(nil)` is a non-nil interface and slips past — `LoggingReporter` logs a typed-nil (and nil-derefs if the concrete type ever grows a `slog.LogValuer` on a nil receiver), and `InMemoryReporter` below silently stores it, which is the opposite of the "nil reports are ignored" the guard advertises. Latent today, but cheap to close before those types land — either a doc line that only untyped nil is skipped, or a reflect `Kind()==Ptr && IsNil()` check. ########## metrics/reporters.go: ########## @@ -0,0 +1,166 @@ +// 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; use [CombineWithLogger] to direct that log elsewhere. +// +// 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 { + return CombineWithLogger(nil, reporters...) +} + +// CombineWithLogger is [Combine] with an explicit logger for recovered reporter +// panics. A nil logger resolves [slog.Default] at each Report call, matching +// Combine, so a later [slog.SetDefault] is honored rather than snapshotted at +// construction. Like Combine, with no non-nil reporters it returns +// [NopReporter] and the logger is unused. +func CombineWithLogger(logger *slog.Logger, 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, logger: logger} +} + +// 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 +} + +var _ Reporter = (*compositeReporter)(nil) + +func (c *compositeReporter) Report(ctx context.Context, report MetricsReport) { + for _, r := range c.reporters { + func() { + defer func() { + if v := recover(); v != nil { + // Swallow per the Reporter contract, but surface the + // failure (with the offending reporter type) so a broken + // reporter is traceable rather than showing up only as + // mysteriously-missing metrics. + logger := c.logger + if logger == nil { + logger = slog.Default() + } + logger.WarnContext(ctx, "iceberg metrics reporter panicked; recovered", Review Comment: Now that we warn on a recovered reporter panic, logging the stack too would make a prod nil-deref actually locatable — right now an operator gets the value and the type but no call site, which is the one thing they'd need to find it. `slog.String("stack", string(debug.Stack()))` here (`debug` = `runtime/debug`) covers it. Take or leave. -- 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]
