Refactor the usage of optional parameters

[pellared]

I think that our continued reliance on these optional parameters is going make the tests harder to maintain. On the one hand, it's nice that the test only needs to call StartTestApplication with a single named parameter. However, based on my experience with these usages, it gets harder and harder to keep track of which parameters are being used over time. That is why I prefer the overloads that take settings/config objects, which make it a lot easier to track down which specific settings are being used/customized.

Originally posted by @nrcventura in https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/pull/900#discussion_r915037182 and https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/pull/918#discussion_r915030492

[pellared]

based on my experience with these usages, it gets harder and harder to keep track of which parameters are being used over time

Do you mean that it is harder to find "unused" named parameters (I mean a case when a non-default value is needed)? I cannot come up with a scenario where the same could not happen if we use a "config object" (instead of optional parameters). I think I need an example 😬

[nrcventura]

What I've run into is something similar to the following. Consider the following code.

MyMethod(bool setting1 = true, bool setting2 = false)
MyMethod(Settings settings)

class Settings
{
    bool Setting1 { get; set; } = true
    bool Setting2 { get; set; } = false
}

Now assume that the MyMethod methods are invoked in a hundred locations, and we need to track down the locations where setting2 is true. In Visual Studio this is really is to do with a property of class (right-click and find usages), but with optional method parameters this is not as easy. You have to find every usage of the method and then look closely to see if the named or ordered parameter is used and then evaluate if the value we care about is being set. The difficulty for tracking down the usage becomes even harder as the number of optional method parameters increase.

[RassK]

I would prefer optional parameters, but they must be always named. It's much easier to search for MyMethod.*setting2:\strue than track the reference trail.

[nrcventura]

I would prefer optional parameters, but they must be always named. It's much easier to search for MyMethod.*setting2:\strue than track the reference trail.

Searching only works when the parameters are always named. Forcing the named usage of these optional parameters is difficult to enforce, because you don't want to always used named parameters for every method. It is much easier to enforce named-usage when you leverage built-in language features that guaranty that you will reference the name. That's one of the advantages of the settings/config objects. In order to set the property you must reference the name either through a property initializer or property assignment. So even if you don't use the IDE's find usages feature, you can still run a code search to find the usages.

Other general concerns with optional parameters are called out in https://medium.com/@thecodewrapper/why-i-hate-optional-parameters-in-c-82bc0d17a2e6 and http://haacked.com/archive/2010/08/10/versioning-issues-with-optional-arguments.aspx/.

[RassK]

Just in case adding it here. Optional parameters cannot be forced to use name with stylecop jet. Seems very old topic. https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/746

[pellared]

I think we can even use methods like SetOtlpMetricsExporter(int port) that would simply call SetEnv*Var*. I think we do nave need optional parameters or other config types. It would simplify test code a lot and remove conditional logic in tests.

We could have test code like this (pseudo code):

collector = StartOtlpMetricsCollector()
collector.Expect("Library.Scope.Name")

InstrumentWithClrProfiler() // requires bytecode instrumentation
SetOtlpMetricsExporter(collector.Port)
RunApplication()

collector.AssertExpectations()

It is a little more code line of codes in the test but I think it is more readable and would not require conditionals based on fields.

@pjanotti FYI

[nrcventura]

@pellared I think that your suggestion of the helper methods will work too, and is easier to read.