Skip to content

Conversation

@j-sund
Copy link
Contributor

@j-sund j-sund commented Apr 10, 2025

Description

Adds a new MetricsReporter implementation to propagate Iceberg table scan metrics into session RuntimeStats

Motivation and Context

Exposing these metrics can improve query observability, which thus makes future improvements to the Iceberg integration a little easier. This also enables better performance monitoring.

Impact

Allows for better visibility into the Iceberg integration.

Test Plan

Ensure the metric is visible and being populated. Basic tests are included in IcebergDistributedSmokeTestBase.java class

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • [] Adequate tests were added if applicable.
  • CI passed.

Release Notes

== NO RELEASE NOTE ==

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 10, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: j-sund / name: Joshua Sundararaman (f09e109)

@j-sund j-sund marked this pull request as ready for review April 12, 2025 03:16
@j-sund j-sund requested review from a team, ZacBlanco and hantangwangd as code owners April 12, 2025 03:16
@j-sund j-sund requested a review from presto-oss April 12, 2025 03:16
@ZacBlanco
Copy link
Contributor

@j-sund Can you sign the CLA using the link in the comment above? You can just sign as a personal contributor.

Also, before your next push try running ./mvnw validate -pl presto-iceberg to ensure your code passes all the checkstyle and license rules. I see the maven checks / maven-checks CI action is failing with the following:

Warning:  Missing header in: /home/runner/work/presto/presto/presto-iceberg/src/main/java/com/facebook/presto/iceberg/RuntimeStatsMetricsReporter.java

@j-sund j-sund force-pushed the runtimeStatsReporter branch from cfc3890 to 67e5b9b Compare April 23, 2025 16:42
Copy link
Contributor

@ZacBlanco ZacBlanco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just minor things. This looks great!

@j-sund j-sund force-pushed the runtimeStatsReporter branch 2 times, most recently from 72ef7e5 to 6124180 Compare May 5, 2025 21:42
@j-sund j-sund force-pushed the runtimeStatsReporter branch 2 times, most recently from 5b96c26 to 8b19e78 Compare May 16, 2025 18:50
Copy link
Member

@hantangwangd hantangwangd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a great feature! Some nits, and a minor problem in the test code.

@hantangwangd
Copy link
Member

I have submit a PR #25140 to make sure that the table name loaded from all catalogs have consistent formatting <catalog>.<schema>.<table>. So I think after that change being merged, we can use the following code to build table name uniformly:

        String catalog = getSession().getCatalog().get();
        String schema = getSession().getSchema().get();
        String tableName = catalog + "." + schema + ".orders";

@hantangwangd
Copy link
Member

Hi @j-sund, since PR #25140 has been merged, you can rebase to include the change. This enables a consistent way to build the table name as described above.

@j-sund j-sund force-pushed the runtimeStatsReporter branch 2 times, most recently from 8e4ba6a to 9ef6b65 Compare June 5, 2025 05:06
@j-sund j-sund requested a review from ZacBlanco June 5, 2025 14:00
Copy link
Contributor

@ZacBlanco ZacBlanco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few style nits. Otherwise LGTM! Can you also squash all of your commits into one?

Copy link
Contributor

@ZacBlanco ZacBlanco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Can you please squash all of your commits into one? Also, try to write a descriptive commit title+message

@j-sund j-sund force-pushed the runtimeStatsReporter branch from bdc7f4b to ff80c23 Compare June 6, 2025 22:22
@j-sund j-sund changed the title RuntimeStatsMetric Reporter for table scans in Iceberg Connector [iceberg] Add metrics reporter for iceberg table scans Jun 6, 2025
@j-sund j-sund force-pushed the runtimeStatsReporter branch from ff80c23 to 55e21d8 Compare June 6, 2025 23:00
Copy link
Member

@hantangwangd hantangwangd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix, LGTM.

@j-sund j-sund requested a review from ZacBlanco June 16, 2025 20:44
@steveburnett
Copy link
Contributor

@j-sund, there have been some fixes to the CI tests since the last activity on this PR. If you rebase, these pending tests might pass.

@j-sund j-sund force-pushed the runtimeStatsReporter branch from 55e21d8 to f09e109 Compare July 7, 2025 13:34
@j-sund
Copy link
Contributor Author

j-sund commented Jul 8, 2025

@steveburnett

maven check failure unable to run 'yarn' for presto-ui.

not sure if this was just a mishap with the github CI runner? My code shouldn't have affected the presto-ui.

Any insight would be appreciated.

@hantangwangd
Copy link
Member

Looks like occasional failures, fixed by a rerun.

Hi @ZacBlanco, do you have any more comments? If not, I think this PR is ready to be merged.

@steveburnett
Copy link
Contributor

@steveburnett

maven check failure unable to run 'yarn' for presto-ui.

not sure if this was just a mishap with the github CI runner? My code shouldn't have affected the presto-ui.

Any insight would be appreciated.

A rerun seems to have worked, at this time I'm seeing "All checks have passed / 108 successful checks".

@j-sund
Copy link
Contributor Author

j-sund commented Aug 7, 2025

@hantangwangd I wanted to check the status of this long standing pr. Im not sure if Zac is an active iceberg maintainer anymore

@hantangwangd hantangwangd dismissed ZacBlanco’s stale review August 7, 2025 17:33

There has been no response for a considerable period of time.

@hantangwangd
Copy link
Member

@j-sund Sorry for the delay of merging. Following the lazy consensus mentioned by @tdcmeehan here, I dismissed ZacBlanco's review. And will merge the PR tomorrow if there are no further objections.

@hantangwangd hantangwangd merged commit 70e1d5e into prestodb:master Aug 8, 2025
169 of 171 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants