tanmayrauth commented on code in PR #1343: URL: https://github.com/apache/iceberg-go/pull/1343#discussion_r3539785255
########## 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: Yeah, the framing was backwards. Reworded the doc now says keeping the marker open is the irreversible choice (sealing it later is what would break), and it's explicit that custom report types are honored only by the in-process reporters (LoggingReporter, InMemoryReporter, Combine). The REST reporter, once it lands, can serialize only the scan-report/commit-report discriminators, so a third-party MetricsReport has no discriminator and is silently dropped there — a spec-compliant catalog rejects it. Added the "don't build a custom report expecting it to reach a catalog endpoint" caveat. Not relitigating open vs sealed — agreed it's right for a client lib. ########## 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: Closed it rather than just documenting. Added an isNilReport helper (reflect Kind()==Ptr && IsNil()) now used by both LoggingReporter and InMemoryReporter, so a (*ScanReport)(nil) is treated as "no report" instead of being logged/stored (and it avoids the nil-receiver LogValuer deref you flagged). Added TestReportersIgnoreTypedNil covering both reporters. ########## 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 now lists NopReporter, LoggingReporter, InMemoryReporter, Combine, and CombineWithLogger, matching 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 { + 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: The warn log now includes slog.String("stack", string(debug.Stack())) alongside the reporter type and panic value, so an operator gets the call site. Added a stack= assertion to the recovered-panic test. -- 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]
