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

[eldimi]

Issue #:
#1274

Description of changes:

Checklist

• Meet tenets criteria](https://docs.powertools.aws.dev/lambda-java/#tenets)
• Update tests
• Update docs
• PR title follows conventional commit semantics

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.

[scottgerring]

Looking good! Some comments inline.

[am29d]

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]

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]

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]

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?

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.

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]

almost OK

[jeromevdl]

@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]

@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]

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]

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]

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]

Is there anything missing from my side here?

[jeromevdl]

LGTM

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[jeromevdl]

@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...

[eldimi]

@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]

@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?