Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refactor AWS client metrics implementation #14327

Merged
merged 2 commits into from
Oct 26, 2022
Merged

Refactor AWS client metrics implementation #14327

merged 2 commits into from
Oct 26, 2022

Conversation

pettyjamesm
Copy link
Member

@pettyjamesm pettyjamesm commented Sep 27, 2022 •
edited
Loading

Description

Refactors AWS client stats for S3 and Glue to to consolidate implementations for core SDK metrics reported by all clients via their RequestMetricCollector. This involves:

  • Moving the overlapping subset of TimeStat / CounterStat / AtomicLong fields from TrinoS3FileSystemStats and GlueMetastoreStats into a new AwsSdkClientCoreStats class, and embedding an instance of that class into each with the @Flatten annotation to preserve the same fields.
  • Dropping AbstractAwsSdkRequestMetricsCollector and separate subclasses for S3 and Glue in favor of a single shared implementation of AwsSdkClientCoreRequestMetricCollector bound to a given AwsSdkClientCoreStats
  • Moving the GlueMetastoreApiStats class to a more general AwsApiCallStats class so that it can be used from other client metrics collectors in the future (none currently implemented in Trino). This is simply a rename and relocation of the class.
  • Reporting TimeStat metrics via the new TimeStat#addNanos method which avoids unnecessary time unit conversions and associated floating point precision loss opportunities.

Non-technical explanation

No non-technical explanation for this change should be necessary.

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Sep 27, 2022
@pettyjamesm pettyjamesm marked this pull request as ready for review September 27, 2022 20:59
@pettyjamesm pettyjamesm requested a review from findepi September 27, 2022 21:23
@findepi findepi requested a review from findinpath September 28, 2022 10:04
findinpath
findinpath reviewed Sep 28, 2022
View reviewed changes
plugin/trino-hive/src/main/java/io/trino/plugin/hive/aws/AwsApiCallStats.java Outdated Show resolved Hide resolved
plugin/trino-hive/src/main/java/io/trino/plugin/hive/s3select/TrinoS3ClientFactory.java Show resolved Hide resolved
plugin/trino-hive/src/main/java/io/trino/plugin/hive/s3select/TrinoS3ClientFactory.java
@@ -109,7 +108,7 @@ private AmazonS3 createS3Client(Configuration config)
AmazonS3Builder<? extends AmazonS3Builder<?, ?>, ? extends AmazonS3> clientBuilder = AmazonS3Client.builder()
.withCredentials(awsCredentialsProvider)
.withClientConfiguration(clientConfiguration)
.withMetricsCollector(new TrinoS3FileSystemMetricCollector(TrinoS3FileSystem.getFileSystemStats()))
Copy link
Contributor

Choose a reason for hiding this comment

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

In case of GlueHiveMetastore the stats field is instantiated per instance, because the metastore can be part of different connectors and the stats can be therefore connected per catalog.

trino/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueHiveMetastore.java

Line 169 in fe608f2

private final GlueMetastoreStats stats = new GlueMetastoreStats();

See the outcome of this design choice in the following test:

trino/testing/trino-product-tests/src/main/java/io/trino/tests/product/deltalake/TestDeltaLakeJmx.java

Line 33 in 6310e5e

row("io.trino.plugin.hive.metastore.cache:name=delta,type=cachinghivemetastore"),

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make sure through a product test that the s3 stats are also collected per catalog?

Copy link
Member Author

Choose a reason for hiding this comment

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

The per-instance instantiation hasn't changed here, so that will be the same as before. Looking around, I don't see any product tests that use a Glue catalog- which is necessary for the JMX object to be registered for such a test. I would add one if it seemed quick, but I'm not sure about how the product test environments get set up or initialized so I would need some help to get the skeleton set up to add a test here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind. By going through the product tests environments, I noticed that the project doesn't have at the moment a product test environment that can be used for AWS tests.

cc @findepi

Copy link
Member

Choose a reason for hiding this comment

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

#5426

@pettyjamesm pettyjamesm force-pushed the consolidate-aws-metrics branch from b4af679 to 0f9b90c Compare September 28, 2022 21:12
@pettyjamesm
Copy link
Member Author

@findepi / @findinpath - are we ok with the current state of the PR to merge it?

@findinpath
Copy link
Contributor

@pettyjamesm I find the implementation clean. Thank you for putting up the PR.

The only thing I'm not keen of is the seemingly unused method in the AwsApiCallStats for which I would find it hard to reason its purpose without having the context of the comments made in this PR. If the PR gets merged, I wouldn't be surprised if the method disappears in a few weeks/months when somebody else stumbling over it will remove it because it is not used anywhere. Let's remove this method if we don't have a use for it in Trino OSS.

@pettyjamesm pettyjamesm force-pushed the consolidate-aws-metrics branch from 0f9b90c to ae2f0fb Compare September 30, 2022 13:28
@pettyjamesm
Copy link
Member Author

@findinpath - ok, rebased and pushed a new commit without the extra method. Let me know if you want anything else changed before you're comfortable approving the PR.

@pettyjamesm pettyjamesm force-pushed the consolidate-aws-metrics branch from ae2f0fb to 15c895b Compare September 30, 2022 20:02
findinpath
findinpath approved these changes Oct 2, 2022
View reviewed changes
Copy link
Contributor

@findinpath findinpath left a comment

Choose a reason for hiding this comment

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

Thank you for the submission and for addressing the comments.

@pettyjamesm pettyjamesm force-pushed the consolidate-aws-metrics branch from 15c895b to 25f5269 Compare October 3, 2022 13:58
@pettyjamesm
Copy link
Member Author

@findepi - any additional changes necessary before merging this PR?

@findepi
Copy link
Member

findepi commented Oct 21, 2022

@alexjo2144 it would be nice if you could help here.

alexjo2144
alexjo2144 approved these changes Oct 24, 2022
View reviewed changes
Copy link
Member

@alexjo2144 alexjo2144 left a comment

Choose a reason for hiding this comment

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

Looks good to me

@alexjo2144
Copy link
Member

I don't think the integration tests around JMX are very good. Mind just confirming that you double checked that the stats are still present with the same names?

@pettyjamesm
Copy link
Member Author

I don't think the integration tests around JMX are very good. Mind just confirming that you double checked that the stats are still present with the same names?

I have indeed confirmed the stats are still present with the same names after this refactor.

@pettyjamesm pettyjamesm force-pushed the consolidate-aws-metrics branch from 25f5269 to d7abf90 Compare October 25, 2022 20:34
@pettyjamesm
Copy link
Member Author

@alexjo2144 - anything else you need in order to merge this PR?

@findepi findepi requested a review from alexjo2144 October 26, 2022 15:18
@findepi findepi merged commit e2c1abd into trinodb:master Oct 26, 2022
@findepi
Copy link
Member

findepi commented Oct 26, 2022

thank you @pettyjamesm @alexjo2144

@pettyjamesm
Copy link
Member Author

Thanks!

@pettyjamesm pettyjamesm deleted the consolidate-aws-metrics branch October 26, 2022 16:02
@github-actions github-actions bot added this to the 401 milestone Oct 26, 2022
@colebow colebow mentioned this pull request Oct 26, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@findinpath findinpath findinpath approved these changes

@alexjo2144 alexjo2144 alexjo2144 approved these changes

@findepi findepi Awaiting requested review from findepi

Assignees
No one assigned
Labels
cla-signed
Milestone
401
Development

Successfully merging this pull request may close these issues.
4 participants
@pettyjamesm @findinpath @findepi @alexjo2144