-
Notifications
You must be signed in to change notification settings - Fork 936
Implement feature IDs for S3 Features #6402
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
Implement feature IDs for S3 Features #6402
Conversation
…nternal/handlers/S3ExpressBusinessMetricInterceptorTest.java
|
* Integration test to verify that S3 Express operations include the correct business metric feature ID | ||
* in the User-Agent header for tracking purposes. | ||
*/ | ||
public class S3ExpressUserAgentIntegrationTest extends S3ExpressIntegrationTestBase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need integ test for this ?
Can we add mocks/stubs for the same test class and move it to Junits to test that request has required user_Agent ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, let's add functional tests using wiremock server or mock http client instead.
|
||
// Verify the User-Agent contains the S3 Express business metric feature ID | ||
String expectedFeatureId = BusinessMetricFeatureId.S3_EXPRESS_BUCKET.value(); | ||
assertThat(userAgent).containsPattern("m/([A-Za-z0-9+\\-]+,)*" + expectedFeatureId + "(,[A-Za-z0-9+\\-]+)*"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we make it as Junit we can assert on deterministic useragen instead of using pattern match , also can we add the final string in verification ?
* Integration test to verify that S3 Access Grants operations include the correct business metric feature ID | ||
* in the User-Agent header for tracking purposes. | ||
*/ | ||
public class S3AccessGrantsUserAgentIntegrationTest extends S3IntegrationTestBase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add Junit and use mock/stubs and verify the request is created with required header ?
*/ | ||
|
||
@SdkInternalApi | ||
public final class S3ExpressUserAgentInterceptor implements ExecutionInterceptor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of separate interceptors , can we create a single interceptor say S3BusinessMetricInterceptor add all the business metric for S3 related in one place else we need to do toBuilder which discards older builder instance and creates new builder instances for every call .
* Interceptor that adds business metrics for S3 Access Grants operations. | ||
*/ | ||
@SdkInternalApi | ||
public final class S3AccessGrantsUserAgentInterceptor implements ExecutionInterceptor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s3accessgrants comes from.
https://github.com/aws/aws-s3-accessgrants-plugin-java-v2/blob/main/src/main/java/software/amazon/awssdk/s3accessgrants/plugin/S3AccessGrantsIdentityProvider.java ,
why can't we modify this package and pass user agent in S3AccessGrants package ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are moving away from using execution interceptor for SDK functionalities. Is there a better way to add user agent other than execution interceptor?
if (request instanceof S3Request) { | ||
S3Request s3Request = (S3Request) request; | ||
|
||
if (S3ExpressUtils.useS3Express(executionAttributes) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is useS3Express and useS3ExpressAuthScheme is used to determine if its S3Express ?
Also since its used in interceptor how do we make sure executionAttributes are rightly set before this interceptor ?
* Interceptor that adds business metrics for S3 Access Grants operations. | ||
*/ | ||
@SdkInternalApi | ||
public final class S3AccessGrantsUserAgentInterceptor implements ExecutionInterceptor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are moving away from using execution interceptor for SDK functionalities. Is there a better way to add user agent other than execution interceptor?
@@ -0,0 +1,2 @@ | |||
software.amazon.awssdk.services.s3.internal.handlers.S3ExpressUserAgentInterceptor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not use execution.interceptors. See #3550
* Integration test to verify that S3 Express operations include the correct business metric feature ID | ||
* in the User-Agent header for tracking purposes. | ||
*/ | ||
public class S3ExpressUserAgentIntegrationTest extends S3ExpressIntegrationTestBase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, let's add functional tests using wiremock server or mock http client instead.
Closing this on behalf of #6409. For S3_ACCESS_GRANTS, since it's a plugin and we are not using execution interceptors, we will create a PR with the team here: https://github.com/aws/aws-s3-accessgrants-plugin-java-v2 |
This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one. |
S3_EXPRESS_BUCKET and S3_ACCESS_GRANTS business metric feature implementation that tracks when an operation called using the S3 Express feature and S3 Access Grants feature.
Motivation and Context
Adding business metrics for feature IDs to track S3 Express (feature ID "J") and S3 Access Grants (feature ID "K") usage in AWS SDK S3 requests. This tracks customer adoption of these S3 features.
Modifications
Added S3_EXPRESS_BUCKET("J") and S3_ACCESS_GRANTS("K") to BusinessMetricFeatureId enum
Created S3ExpressUserAgentInterceptor that detects when the bucket name matches S3 Express Ruleset and express credentials are used.
Created S3AccessGrantsUserAgentInterceptor that detects S3 Access Grants plugin by checking if providerName contains "S3AccessGrantsIdentityProvider"
Registered both interceptors in S3's execution.interceptors
When S3 Express sessions detected, adds metric "J" to business metrics collection, appearing as "m/J" in User-Agent headers
When S3 Access Grants plugin detected, adds metric "K" to business metrics collection, appearing as "m/K" in User-Agent headers
Testing
Added unit tests and integ tests.
Screenshots (if appropriate)
Types of changes
Checklist
mvn install
succeedsscripts/new-change
script and following the instructions. Commit the new file created by the script in.changes/next-release
with your changes.License