10000 chore: Add powertools specific user-agent-suffix to the AWS SDK v2 clients by eldimi · Pull Request #1306 · aws-powertools/powertools-lambda-java · GitHub
[go: up one dir, main page]
Skip to content

chore: Add powertools specific user-agent-suffix to the AWS SDK v2 clients #1306

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
Merged
merged 11 commits into from
Aug 2, 2023
Merged

chore: Add powertools specific user-agent-suffix to the AWS SDK v2 clients #1306

merged 11 commits into from
Aug 2, 2023

Conversation

eldimi
Copy link
Contributor
@eldimi eldimi commented Jul 21, 2023

Issue #:
#1274

Description of changes:

Checklist

Breaking change checklist

RFC issue #:

  • Migration process documented
  • Implement warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@eldimi eldimi marked this pull request as ready for review July 21, 2023 14:02
am29d
am29d reviewed Jul 21, 2023
View reviewed changes
am29d
am29d reviewed Jul 21, 2023
View reviewed changes
scottgerring
scottger 10000 ring reviewed Jul 21, 2023
View reviewed changes
Copy link
Contributor
@scottgerring scottgerring left a comment

Choose a reason for hiding this comment

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

Looking good! Some comments inline.

am29d
am29d reviewed Jul 21, 2023
View reviewed changes
Copy link
Contributor
@am29d am29d left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks great! I only have two questions:

  1. Can you also instrument clients that customer pass to the utility? Atm it only happens in if (client == null) blocks.

  2. Is it feasible to extract overrideConfiguration in a dedicated place? Atm every client runs exactly the same line. Might be easier to have it in one place and apply changes in the future in one file, instead of going through every constructor.

@jeromevdl
Copy link
Contributor
jeromevdl commented Jul 21, 2023 •
edited
Loading

Looks great! I only have two questions:

  1. Can you also instrument clients that customer pass to the utility? Atm it only happens in if (client == null) blocks.
  2. Is it feasible to extract overrideConfiguration in a dedicated place? Atm every client runs exactly the same line. Might be easier to have it in one place and apply changes in the future in one file, instead of going through every constructor.

Overall looks good but I agree with Alex, we should add the user agent even on clients they pass to our modules. And +1 on the second point: can we have a method in the core module where we can pass any client, we override the configuration and return the client ?

And the build is failing, not sure about the logger you use... (sun ? see Scott's comment about that)

@codecov-commenter
Copy link
codecov-commenter commented Jul 22, 2023 •
edited
Loading

Codecov Report

Patch coverage: 62.22% and project coverage change: -0.29% ⚠️

Comparison is base (6900b72) 79.35% compared to head (8f0e137) 79.06%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #1306      +/-   ##
============================================
- Coverage     79.35%   79.06%   -0.29%     
- Complexity      641      651      +10     
============================================
  Files            73       74       +1     
  Lines          2446     2489      +43     
  Branches        253      257       +4     
============================================
+ Hits           1941     1968      +27     
- Misses          425      441      +16     
  Partials         80       80
Files Changed Coverage Δ
...ambda/powertools/parameters/AppConfigProvider.java 86.88% <0.00%> (-2.95%) ⬇️
...zon/lambda/powertools/parameters/BaseProvider.java 97.91% <ø> (ø)
...lambda/powertools/parameters/DynamoDbProvider.java 83.05% <0.00%> (-1.44%) ⬇️
.../lambda/powertools/parameters/SecretsProvider.java 70.73% <0.00%> (-1.77%) ⬇️
...oftware/amazon/lambda/powertools/sqs/SqsUtils.java 72.41% <9.09%> (-8.36%) ⬇️
...mpotency/persistence/DynamoDBPersistenceStore.java 90.44% <50.00%> (-0.53%) ⬇️
...owertools/core/internal/UserAgentConfigurator.java 92.59% <92.59%> (ø)
...azon/lambda/powertools/parameters/SSMProvider.java 89.70% <100.00%> (+0.15%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@eldimi
Copy link
Contributor Author
eldimi commented Jul 26, 2023

Looks great! I only have two questions:

  1. Can you also instrument clients that customer pass to the utility? Atm it only happens in if (client == null) blocks.

This can happen if we implement the ExecutionInterceptor and modify the request headers, since we can't modify the sdk client as soon as it is built. If we go this way, maybe it makes sense to do it in every case (whether the client already exists or not) so that we have a common implementation. What do you think?

  1. Is it feasible to extract overrideConfiguration in a dedicated place? Atm every client runs exactly the same line. Might be easier to have it in one place and apply changes in the future in one file, instead of going through every constructor.

I thought about doing that initially. But then, I thought that it might be better if we do a refactoring(in a seperate PR) and have a common way of building clients. This is because, duplicate code is also the issue when setting the region and the HttpClient. They are set is several places, the same way. So why extract only the overrideConfguration part? And if we modify something else in the future (either in overrideConfiguration or somewhere else), where should it be placed?
Thinking out loud here :)

Overall looks good but I agree with Alex, we should add the user agent even on clients they pass to our modules. And +1 on the second point: can we have a method in the core module where we can pass any client, we override the configuration and return the client ?

And the build is failing, not sure about the logger you use... (sun ? see Scott's comment about that)

Yes, fixed that already, had accidentaly a wrong import and I didn't notice at all that was the case

@am29d @jeromevdl See my comments inline

jeromevdl
jeromevdl reviewed Jul 26, 2023
View reviewed changes
jeromevdl
jeromevdl reviewed Jul 26, 2023
View reviewed changes
Copy link
Contributor
@jeromevdl jeromevdl left a comment

Choose a reason for hiding this comment

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

almost OK

@jeromevdl
Copy link
Contributor

@eldimi I agree we should refactor this and have the common sdk client stuff in the core module ("common" in v2). Would you like to do this? (in another PR)

@eldimi
Copy link
Contributor Author
eldimi commented Jul 27, 2023

@eldimi I agree we should refactor this and have the common sdk client stuff in the core module ("common" in v2). Would you like to do this? (in another PR)

yes, I can do that

@eldimi
Copy link
Contributor Author
eldimi commented Jul 27, 2023

almost OK

I saw some code smells reported by sonarqube - related to the current PR changes. I will fix them too in a next commit

@eldimi
Copy link
Contributor Author
eldimi commented Jul 27, 2023
  1. instrument clients that customer

almost OK

What abou instrumenting the clients that customers provide? Any comments on that related to my reply here?

@jeromevdl
Copy link
Contributor
  1. instrument clients that customer

almost OK

What abou instrumenting the clients that customers provide? Any comments on that related to my reply here?

I agree, should be part of the refactor I mentioned. We should first merge this PR and then you move all the common stuff, plus the interceptor somewhere.

@eldimi
Copy link
Contributor Author
eldimi commented Aug 2, 2023

Is there anything missing from my side here?

jeromevdl
jeromevdl approved these changes Aug 2, 2023
View reviewed changes
Copy link
Contributor
@jeromevdl jeromevdl left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarqubecloud
Copy link
sonarqubecloud bot commented Aug 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@jeromevdl
Copy link
Contributor

@eldimi, I'll merge that one. Can you write an RFC for the instrumentation and generic client stuff you proposed? So we can elaborate on it...

@jeromevdl jeromevdl merged commit 7179713 into aws-powertools:main Aug 2, 2023
@eldimi
Copy link
Contributor Author
eldimi commented Aug 2, 2023

@eldimi, I'll merge that one. Can you write an RFC for the instrumentation and generic client stuff you proposed? So we can elaborate on it...

sure, I'll do that

@eldimi
Copy link
Contributor Author
eldimi commented Aug 4, 2023 •
edited
Loading

@eldimi, I'll merge that one. Can you write an RFC for the instrumentation and generic client stuff you proposed? So we can elaborate on it...

@jeromevdl Looking a bit more into that..The ExecutionInterceptor is only configured in the builder, therefore no way that it can be used to intercept the request for an already provided client instance.
Regarding the refactoring for moving the common sdk client initialization in the core module, do we still need an RFC for that? Also, if we are implementing #1092, maybe it could be part of that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scottgerring scottgerring scottgerring left review comments

@am29d am29d am29d left review comments

@jeromevdl jeromevdl jeromevdl approved these changes

Assignees

@eldimi eldimi

Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@eldimi @jeromevdl @codecov-commenter @scottgerring @am29d
0