feat: metrics reporting for scan and commit#589
Conversation
evindj
force-pushed
the
metrics_core_components
branch
from
5fa02cb to
0a85180
Compare
|
Let me know when ready for review :) |
for sure ! most likely tomorrow |
evindj
force-pushed
the
metrics_core_components
branch
from
eaacbe8 to
5241044
Compare
evindj
force-pushed
the
metrics_core_components
branch
from
5241044 to
d26fb96
Compare
|
Thanks for updating this! I still need to take some time to get familiar with the Java implementation and its API in the rest spec before reviewing it. |
wgtmac
left a comment
wgtmac
left a comment
There was a problem hiding this comment.
Thanks for working on this! I have some general questions on the design, especially on the capability of customization. IMO we can focus on the API design for now and later integrate with other classes. Please let me know what you think.
| struct ICEBERG_EXPORT ScanReport { | ||
| /// \brief The fully qualified name of the table that was scanned. | ||
| std::string table_name; | ||
|
|
There was a problem hiding this comment.
Why some metrics have blank lines in between but others don't? I think we can remove all these blank lines to be compact.
| int64_t snapshot_id = -1; | ||
|
|
||
| /// \brief Filter expression used in the scan, if any. | ||
| std::string filter; |
There was a problem hiding this comment.
Should this be std::shared_ptr<Expression>?
| namespace iceberg { | ||
|
|
||
| /// \brief Duration type for metrics reporting in milliseconds. | ||
| using DurationMs = std::chrono::milliseconds; |
There was a problem hiding this comment.
This hard-codes our reported duration unit to be milliseconds, which violates the spec I think?
| std::string filter; | ||
|
|
||
| /// \brief Schema ID. | ||
| int32_t schema_id = -1; |
There was a problem hiding this comment.
We are missing some fields like projectedFieldIds and projectedFieldNames from the Java implementation. I think they are required by the REST spec: https://github.com/apache/iceberg/blob/149cc464f9b7df800cc5718af725983473819504/open-api/rest-catalog-open-api.yaml#L3990-L4023
| std::string table_name; | ||
|
|
||
| /// \brief The snapshot ID created by this commit. | ||
| int64_t snapshot_id = -1; |
There was a problem hiding this comment.
Please use kInvalidSnapshotId defined in the iceberg/constant.h
| /// \brief Total number of data manifests. | ||
| int64_t total_data_manifests = 0; | ||
|
|
||
| /// \brief Number of data manifests that were skipped. |
| int64_t skipped_data_files = 0; | ||
|
|
||
| /// \brief Number of data manifests that were skipped. | ||
| int64_t skipped_delete_files = 0; |
| int32_t schema_id = -1; | ||
|
|
||
| /// \brief Total duration of the entire scan operation. | ||
| DurationMs total_duration{0}; |
There was a problem hiding this comment.
Should we remove this as this is not defined by the Java implementation?
| /// | ||
| /// This variant type allows handling both report types uniformly through | ||
| /// the MetricsReporter interface. | ||
| using MetricsReport = std::variant<ScanReport, CommitReport>; |
There was a problem hiding this comment.
If we define MetricsReport as a std::variant, we cannot support customizing metrics report. For example, engines may have more metrics to report than defined by the Java implementation. Even the REST spec does not explicitly define what keys are required.
Instead, should we define the MetricsReport like below?
struct MetricsReport {
std::string kind; // can be "scan" or "commit", or whatever customized
std::unordered_map<std::string, CounterResult> counter_results;
std::unordered_map<std::string, TimerResult> timer_results;
};What do you think?
There was a problem hiding this comment.
After thinking more on this, I'm fine to use your current approach to define ScanReport and CommitReport with explicit fields. MetricsReports are collected by this library so users do not have the flexibility to customize them. We only need to customize MetricsReporter.
There was a problem hiding this comment.
I like the bag of keys approach because it provides for customization without necessarily having to require compilation. let me think a bit more about the entire thing, I might reach out on slack for a quick sync.
| /// | ||
| /// \param reporter_type Case-insensitive type identifier (e.g., "noop"). | ||
| /// \param factory Factory function that produces the reporter. | ||
| static void Register(std::string_view reporter_type, MetricsReporterFactory factory); |
There was a problem hiding this comment.
How do we support the Java parity CompositeMetricsReporter? It would be useful if we want to report metrics to multiple sinks.
This is fair, I just wanted to show the end to end picture for ease of understanding. |
2 participants
Initial commit for addressing #533