-
Notifications
You must be signed in to change notification settings - Fork 367
feat(metrics): Evolve PolarisMetricsReporter interface with timestamp parameter and comprehensive documentation #3468
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
132ca9b
0bc647e
aa43beb
489a140
ea47412
772e6fb
f3bd22e
194150c
e4e9f0b
d69c08a
f2e2379
64b52e0
cbeb620
1a3c2b2
a7c8363
bd15dcd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,9 +18,39 @@ | |
| */ | ||
| package org.apache.polaris.service.reporting; | ||
|
|
||
| import java.time.Instant; | ||
| import org.apache.iceberg.catalog.TableIdentifier; | ||
| import org.apache.iceberg.metrics.MetricsReport; | ||
|
|
||
| /** | ||
| * SPI interface for reporting Iceberg metrics received by Polaris. | ||
| * | ||
| * <p>Implementations can be used to send metrics to external systems for analysis and monitoring. | ||
| * Custom implementations can be annotated with appropriate {@code Quarkus} scope and {@link | ||
| * io.smallrye.common.annotation.Identifier @Identifier("my-reporter-type")} for CDI discovery. | ||
| * | ||
| * <p>The implementation to use is selected via the {@code polaris.iceberg-metrics.reporting.type} | ||
| * configuration property, which defaults to {@code "default"}. | ||
| * | ||
| * <p>Implementations can inject other CDI beans for context. | ||
| * | ||
| * @see DefaultMetricsReporter | ||
| * @see MetricsReportingConfiguration | ||
| */ | ||
| public interface PolarisMetricsReporter { | ||
| public void reportMetric(String catalogName, TableIdentifier table, MetricsReport metricsReport); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we release this public interface in 1.3 right ? i wonder if we keep this api with default impl to the new method with null or something ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @singhpk234 I had a deprecated backward compatible method and the consensus was to remove it since this SPI is not meant to be used by developers outside Polaris.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't fully get it, wdym by developers outside Polaris ? can this not be used downstream projects, my recommendation is to make sure version upgrades are seemless so keep this and add more if we want and just update the default of this work under timestamp null assumption.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me also check previous discussions, can you please point me to that ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per our standing evolution guidelines We should and do try to make java API changes in a backward -compatible manner when practical. Regarding this particular PR and java interfaces that are part of an SPI (defined and called by Polaris, implemented by 3rd party plugins), I do not see a practical way to evolve them in a backward-compatible manner without causing excessive maintenance burden in Polaris code. In this case, Polaris would have to define a new interface for the new method signature and perform That said, I'd like to propose moving the SPI evolution discussion to the @singhpk234 : please clarify whether you consider this comment thread a blocker for merging (or not).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dimas-b can we not add the api in the same interface and add the default impl of old api call this null clock, i would like to understand this more if this is possible. If its not possible i am supportive of the change !
thanks for asking the clarification i believe anything is thats not marked as 'nit' / 'not blocker' / 'not' is expected to be resolved before merging (per here) , i really appreciate you checking in !
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I do not see a point in adding a We could add a Please note that providing a custom implementation for this interface requires a downstream build. So all upgrades will go through local builds and CI (assuming normal software engineering practices), where the need for adjustments will be become apparent.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I uptook 1.3.0, I was glad that my compilation failed since if the old method was deprecated, I would be confused why I need to uptake the new approach and how to do it. Having said that, this PR started as adding events for metrics, we removed it. It only contains this extra attribute. It is not a blocker for me. If we don't agree on how to proceed, I would rather abandon this PR and focus on the simplified metrics persistence code here: #3385. That allows Polaris to persist table metrics to the database.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thats fair, though in this case we are forcing the upgrades to have this new interface implementation when having the old api was not hurting ? taking the case LOGGERs can be configured to implicitly log timestamp additionally and the way we have reporting wired if i wanna do custom one can just do timestamp internally rather than requesting from the signature ? it not the time when the request hit the server but its the time when the reportMetric is called so does it matter of i do this vs reportMetric(catalogName, tableIdentifier, reportMetricsRequest.report())) { with that being said i think its fine if we wanna move forward :), but above is my thought process. Please move forward @dimas-b trust your judgement here ! |
||
|
|
||
| /** | ||
| * Reports an Iceberg metrics report for a specific table. | ||
| * | ||
| * @param catalogName the name of the catalog containing the table | ||
| * @param table the identifier of the table the metrics are for | ||
| * @param metricsReport the Iceberg metrics report (e.g., {@link | ||
| * org.apache.iceberg.metrics.ScanReport} or {@link org.apache.iceberg.metrics.CommitReport}) | ||
| * @param receivedTimestamp the timestamp when the metrics were received by Polaris | ||
| */ | ||
| void reportMetric( | ||
| String catalogName, | ||
| TableIdentifier table, | ||
| MetricsReport metricsReport, | ||
| Instant receivedTimestamp); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.