[Instrumentation.AWSLambda] Remove AddAWSLambdaConfigurations with default parameter

[Kielek]

Propagate recommendations from https://github.com/open-telemetry/opentelemetry-dotnet/pull/3653/files#r970049254

Changes

Remove AddAWSLambdaConfigurations with default parameter

For significant contributions please make sure you have completed the following items:

• Appropriate CHANGELOG.md updated for non-trivial changes
• [ ] Design discussion issue #

[codecov]

Codecov Report

Merging #943 (858d37c) into main (7fda8d4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 858d37c differs from pull request most recent head 27f6230. Consider uploading reports for the commit 27f6230 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #943   +/-   ##
=======================================
  Coverage   70.18%   70.18%           
=======================================
  Files         210      210           
  Lines        7864     7865    +1     
=======================================
+ Hits         5519     5520    +1     
  Misses       2345     2345           

Impacted Files	Coverage Δ
...ation.AWSLambda/TracerProviderBuilderExtensions.cs	100.00% <100.00%> (ø)

[Oberon00]

What is the advantage of this change? It seems to make the code longer and less clear and is also API-breaking. I also don't see the relation to the linked comment, but probably I'm missing something?

[Kielek]

@Oberon00, I have linked wrong comment, see https://github.com/open-telemetry/opentelemetry-dotnet/pull/3653/files#r969915942

If some user was binding to this reflectively and specifically looking at metadata for the optional parameter they would no longer find it and break but I don't think that is really worth worrying about. The benefit to this change is to keep consistency with everything else and allow for future safe expansion/overload additions without default parameter ambiguity issues.

In general it is binary-breaking change but does not require any source code changes.
As long as the package is in pre-GA phase it is safe to merge IMO.

[Oberon00]

Can you maybe help me with some link to an explanation of this best practice of avoiding default parameters (assuming it is one)?

[Oberon00]

This is confusing because from the user's perspective it is not removed but just replaced with another method. Probably we don't even need a changelog entry here, unless it's really ABI-breaking

[Kielek]

In some strange cases it can be treated as ABI-breaking. https://stackoverflow.com/a/9916197
Most cases it is not.

[Kielek]

Can you maybe help me with some link to an explanation of this best practice of avoiding default parameters (assuming it is one)?

I do not found anything beyond https://github.com/open-telemetry/opentelemetry-dotnet/pull/3653/files#r969915942.

For me, follow the rule from SDK repository is good enough argument to this change.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[Oberon00]

OK with me then, although I'm not entirely convinced, I don't want to block it either, because I do acknowledge that I might just be ignorant of the value of this change.

So the rule boils down to: "Never combine overloads and default arguments"? I.e. if I have an overloaded method, I must never add default arguments to any overload, and if I have default arguments I must never overload the method?

[utpilla]

So the rule boils down to: "Never combine overloads and default arguments"? I.e. if I have an overloaded method, I must never add default arguments to any overload, and if I have default arguments I must never overload the method?

@Oberon00 Not having optional arguments in public methods makes it simpler to extend the methods to accept more arguments in future without worrying about making breaking changes. This doc talks about it in detail:

https://github.com/dotnet/roslyn/blob/main/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md

We agreed that it's best to not have optional parameters in the public methods. This allows for a very easy way to change the method signature if required in future.