Opentelemetry-dotnet: More cops (FxCop)

[reyang]

Currently we have Stylecop but no FxCop, this is to add FxCop based on the feedback here.

[eddynaka]

@reyang , i was looking at this and i have one question about our current project structure.
Right now, we have the following folders with code:

• examples -> Common.nonprod -> Common.props
• src -> Common.prop -> Common.props
• test -> Common.nonprod -> Common.props

Right now, we have the StyleCop in Common.props. So, it's analyzing all projects inside examples, src, test. But, if we add the same to FxCop, we will receive some errors from examples code that I think is not a priority.

What do you suggest? Should we create another props? Is it possible to create a condition based on the folder? To enable only for the src, for example. Or, should we solve everything?

[reyang]

I guess at this stage we only need fxcop in Common.prod.props.

[CodeBlanch]

Agree the priority is to get it into the main source. I think second priority is actually the examples, so that we provide best-practice stuff to our future friends. Third is tests. Fourth is happy hour.

[eddynaka]

I was looking at the current errors that FxCop shows, below a list:

• CA1031: should we disable it? Since every exception we are going to save it and not rethrow
• CA1066/CA1805/CA1815: should we disable it?
• CA1303: should we create a resource? Are we going to support multiple language?

Should we really do this?

• CA1822: we have some places that we can change to static method. Should we change?
• CA2007: what about ConfigureAwait? should we call it everywhere with false?

[CodeBlanch]

My thoughts...

Turn off (set to None via ruleset):

• CA1303 (localization): We can always turn on if we want to support multiple languages?
• CA1031 (general exception catch): I usually leave it on just to be explicit but you make a good case for turning it off. Fine by me if other people agree.

Leave on:

• CA1066/CA1805/CA1815 (value type comparison & equality): The IDE has some quick refactorings now that make it really easy to implement those operations. It guarantees expectations users might have with how our public structs work relative to each other. Think about something like DateTime. You just expect certain comparisons to work on that. Should you expect SpanContextA == SpanContextB to work? Probably?
• CA1822 (make things static): It's basically for performance, I think. IL.Call vs IL.Callvirt. Good to do where we can. Some things that are part of public API we might want to keep instance-based, but you can always suppress the warning with pragma.
• CA2007 (configureAwait): I think the general guidance is libraries should always use .ConfigureAwait(false)? So the warning is valid IMO.

[eddynaka]

Ok, after those PR's, we still have some issues to solve (~100).

The one's that I'm not sure the overhead and if we should do it:

• CA1305: what would be the overhead to use ToString and add the InvariantCulture option
• CA1307: what would be the overhead to compare the strings using StringComparison.IgnoreCulture or other option.

What do you think?

[CodeBlanch]

@eddynaka I'm not 100% certain, but I feel like there would actually be slightly less overhead if we specified the format/culture to use. Because if we don't specify it, the framework has to go and look up the current culture. Probably OK to just use invariant everywhere. @reyang What do you think?

[reyang]

I think in this project we should use either StringComparison.Ordinal or StringComparison.OrdinalIgnoreCase, depending on the scenario.

We need to use ToString with InvariantCulture everywhere as we're not dealing with UI (e.g. we don't expect an internal exception to be translated to Chinese; we don't expect 12345 to be formatted to "12,345").

[eddynaka]

I think in this project we should use either StringComparison.Ordinal or StringComparison.OrdinalIgnoreCase, depending on the scenario.

We need to use ToString with InvariantCulture everywhere as we're not dealing with UI (e.g. we don't expect an internal exception to be translated to Chinese; we don't expect 12345 to be formatted to "12,345").

@CodeBlanch @reyang , i will open a separate pr (probably part 06), because part 05 already has ~40 files changes.