Conversation
|
|
There are some metrics that ice is not currently tracking CommitMetrics |
shyiko
left a comment
shyiko
left a comment
There was a problem hiding this comment.
Looks good overall. Thank you!
ice/src/main/resources/logback.xml
Outdated
There was a problem hiding this comment.
isn't this an indicator that we're not doing a good job at closing input streams and need to fix that?
There was a problem hiding this comment.
This was coming from iceberg API and it was flooding the logs in a loop, I will get the exception
There was a problem hiding this comment.
removed, not able to reproduce it, will add in a separate PR.
There was a problem hiding this comment.
nit: comments like these should be avoided
| private final Counter messageParseErrorsTotal; | ||
|
|
||
| /** Returns the singleton instance of the metrics reporter. */ | ||
| public static InsertWatchMetrics getInstance() { |
There was a problem hiding this comment.
nit: why not use IoDH for instance lazy loading (and avoid the lock?
There was a problem hiding this comment.
should we perhaps drop sqs from metric names here and below for consistency with metrics above + in case we decide to add support for other message queue?
There was a problem hiding this comment.
Updated to include queue as a filter for grafana dashboard.
examples/scratch/README.md
Outdated
There was a problem hiding this comment.
any reason it's not metrics.recordOrphanDeleteFailure(tableName, failedCount.get())?
| } | ||
|
|
||
| // Initialize Iceberg metrics reporter for Prometheus (singleton) | ||
| PrometheusMetricsReporter metricsReporter = PrometheusMetricsReporter.getInstance(); |
There was a problem hiding this comment.
some Metrics objects are passed around (like here), others are not (like MaintenanceMetrics.getInstance()). Let's settle on one approach
There was a problem hiding this comment.
Changed the ones passed to RestCatalogAdapter to use singleton and tested the changes.
There was a problem hiding this comment.
throw instead of ignore-logging, perhaps?
There was a problem hiding this comment.
I'd suggest introducing cli option (e.g. --watch-endpoint) that allow to specify a different value from --watch as proposed approach falls apart when using localstack, etc. This way things also can continue to use AWS_ENDPOINT_URL_SQS env var when needed.
There was a problem hiding this comment.
examples/s3watch/test/README.md
Outdated
There was a problem hiding this comment.
this needs to be updated, right?
|
d8a655c should not be included in this PR as it's unrelated to the rest of the changes |
removed. |
|
Added Catalog-level: iceberg_catalog_tables_total (gauge) snapshots_count |
xieandrew
left a comment
xieandrew
left a comment
There was a problem hiding this comment.
Issues I found during testing:
- Tables created by insert watch don't increment the iceberg_catalog_tables (count of all tables) metric
- The "Scan Metrics" row in Grafana is empty. The corresponding panels are under "Overview" instead.
…_metrics # Conflicts: # ice-rest-catalog/src/main/java/com/altinity/ice/rest/catalog/Main.java # ice/src/main/java/com/altinity/ice/cli/Main.java # ice/src/main/java/com/altinity/ice/cli/internal/cmd/InsertWatch.java
3 participants
No description provided.