chore: Add powertools specific user-agent-suffix to the AWS SDK v2 clients #1306
Conversation
...-core/src/main/java/software/amazon/lambda/powertools/core/internal/UserAgentConfigurer.java
Outdated
Show resolved
Hide resolved
...e/src/test/java/software/amazon/lambda/powertools/core/internal/UserAgentConfigurerTest.java
Outdated
Show resolved
Hide resolved
...java/software/amazon/lambda/powertools/idempotency/persistence/DynamoDBPersistenceStore.java
Outdated
Show resolved
Hide resolved
...-core/src/main/java/software/amazon/lambda/powertools/core/internal/UserAgentConfigurer.java
Outdated
Show resolved
Hide resolved
...-core/src/main/java/software/amazon/lambda/powertools/core/internal/UserAgentConfigurer.java
Outdated
Show resolved
Hide resolved
...-core/src/main/java/software/amazon/lambda/powertools/core/internal/UserAgentConfigurer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Looks great! I only have two questions:
-
Can you also instrument clients that customer pass to the utility? Atm it only happens in
if (client == null)blocks. -
Is it feasible to extract
overrideConfigurationin a dedicated place? Atm every client runs exactly the same line. Might be easier to have it in one place and apply ch 8000 anges 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 ReportPatch coverage:
❗ 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
☔ View full report in Codecov by Sentry. |
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?
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?
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 |
powertools-core/src/main/resources-filtered/version.properties
Outdated
Show resolved
8000
Hide resolved
...-parameters/src/main/java/software/amazon/lambda/powertools/parameters/DynamoDbProvider.java
Outdated
Show resolved
Hide resolved
|
@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 |
I saw some code smells reported by sonarqube - related to the current PR changes. I will fix them too in a next commit |
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. |
|
Is there anything missing from my side here? |
|
Kudos, SonarCloud Quality Gate passed!
|
|
@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 |
@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. |
Issue #:
#1274
Description of changes:
Checklist
Breaking change checklist
RFC issue #:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.