Enable analysers #908

martincostello · 2023-01-19T12:21:24Z

Changes

Enable <AnalysisLevel>latest-All</AnalysisLevel> as suggested in #900 (comment).

Within the production shipping packages there are two changes that change the public API surface. As both packages are still pre-release on NuGet.org I assumed this was fine. These changes can be reverted and suppressed if preferred.

For the test projects I erred on the side of disabling rules rather than fixing them to reduce churn.

I'm leaving this as a draft for now as it touches a lot of files so might ping a lot of different reviewers. I can also split the commits up differently and/or create smaller more targeted PRs if that's preferred.

Let me know if any of these changes warrant a note in the CHANGELOG.

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

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

Set `AnalysisLevel` to `latest-All`.

Fix (or suppress) code analysis warnings for `OpenTelemetry.Contrib.Extensions.AWSXRay`.

Fix (or suppress) code analysis warnings for `OpenTelemetry.Contrib.Instrumentation.AWS`.

Fix (or suppress) code analysis warnings for `OpenTelemetry.Contrib.Shared`.

Fix tests broken by 65e7e03.

Fix (or suppress) code analysis warnings for `OpenTelemetry.Exporter.Instana`.

Fix (or suppress) code analysis warnings for `OpenTelemetry.Exporter.Stackdriver`.

Fix (or suppress) code analysis warnings for `OpenTelemetry.Extensions.Docker`.

ix (or suppress) code analysis warnings for `OpenTelemetry.Extensions.PersistentStorage`.

Fix (or suppress) code analysis warnings for `OpenTelemetry.Instrumentation.AspNet`.

Fix (or suppress) code analysis warnings for `OpenTelemetry.Instrumentation.AWSLambda`.

Fix (or suppress) code analysis warnings for `OpenTelemetry.Instrumentation.ElasticsearchClient`.

Fix (or suppress) code analysis warnings for `OpenTelemetry.Instrumentation.EntityFrameworkCore`.

Fix (or suppress) code analysis warnings for `OpenTelemetry.Instrumentation.GrpcCore`.

Fix (or suppress) code analysis warnings for `OpenTelemetry.Contrib.Extensions.AWSXRay`.

Fix (or suppress) code analysis warnings for `OpenTelemetry.Instrumentation.MySqlData`.

Fix (or suppress) code analysis warnings for `OpenTelemetry.Contrib.Extensions.AWSXRay`.

Fix (or suppress) code analysis warnings for `OpenTelemetry.Instrumentation.Wcf`.

Use `Guard.ThrowIfNull()` where possible.

- Fix analysis warnings in non-production projects, such as tests and examples. - Fix analysis warnings in production projects missed in the first pass.

Fix CA1725 warning by adjusting the public API.

Fix test broken by refactor to use `TryGetValue()`.

codecov · 2023-01-19T12:37:33Z

Codecov Report

Merging #908 (1c7b54f) into main (b53b6b9) will decrease coverage by 0.26%.
The diff coverage is 82.06%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #908      +/-   ##
==========================================
- Coverage   68.48%   68.22%   -0.26%     
==========================================
  Files         183      183              
  Lines        7002     7018      +16     
==========================================
- Hits         4795     4788       -7     
- Misses       2207     2230      +23

Impacted Files	Coverage Δ
...y.Contrib.Extensions.AWSXRay/AWSXRayIdGenerator.cs	`61.64% <ø> (ø)`
...orter.Geneva/TLDExporter/UncheckedASCIIEncoding.cs	`16.00% <0.00%> (+0.31%)`	⬆️
...Implementation/Processors/ActivityProcessorBase.cs	`92.85% <0.00%> (ø)`
...metry.Exporter.Instana/Implementation/Transport.cs	`0.00% <0.00%> (-18.30%)`	⬇️
...xporter.Instana/TracerProviderBuilderExtensions.cs	`0.00% <ø> (ø)`
...y.Exporter.Stackdriver/Implementation/Constants.cs	`0.00% <0.00%> (-100.00%)`	⬇️
...ter.Stackdriver/TracerProviderBuilderExtensions.cs	`100.00% <ø> (ø)`
...elemetry.Exporter.Stackdriver/Utils/CommonUtils.cs	`0.00% <0.00%> (ø)`
...y.Extensions.PersistentStorage/FileBlobProvider.cs	`77.41% <ø> (ø)`
....AspNet.TelemetryHttpModule/TelemetryHttpModule.cs	`5.26% <0.00%> (-0.15%)`	⬇️
... and 54 more

Oberon00 · 2023-01-19T12:55:27Z

build/Common.props

@@ -7,6 +7,7 @@
    <DefineConstants>$(DefineConstants);SIGNED</DefineConstants>
    <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>
    <NetFrameworkMinimumSupportedVersion>net462</NetFrameworkMinimumSupportedVersion>
+    <AnalysisLevel>latest-All</AnalysisLevel>


latest-All sound a bit too much, can we use a fixed version here?

There is an agreement to use this approach on SDK repository. I think we could do the same here. See open-telemetry/opentelemetry-dotnet#3958

Makes sense.

Oberon00 · 2023-01-19T12:55:54Z

src/OpenTelemetry.Contrib.Extensions.AWSXRay/Resources/AWSEKSResourceDetector.cs

@@ -41,19 +41,19 @@ public class AWSEKSResourceDetector : IResourceDetector
    public IEnumerable<KeyValuePair<string, object>> Detect()
    {
        var credentials = this.GetEKSCredentials(AWSEKSCredentialPath);
-        var httpClientHandler = Handler.Create(AWSEKSCertificatePath);
+        using var httpClientHandler = Handler.Create(AWSEKSCertificatePath);


Oberon00 · 2023-01-19T12:57:06Z

All in all, this PR seems a bit too large. The nontrivial changes should probably go to separate PRs (separately dis/enabled before)

Kielek · 2023-01-19T13:11:46Z

All in all, this PR seems a bit too large. The nontrivial changes should probably go to separate PRs (separately dis/enabled before)

I think that enabling this feature project by project (including tests) could be easier to review. If you agree, it will be great to create issue to link all changes, similar to #894.

@TimothyMothra, fyi as you working on similar issue in SDK repository.

Oberon00 · 2023-01-19T13:14:07Z

I was thinking of doing the trivial warnings (such as adding static) in one PR, and for the more involved changes, doing it per warning, or per project, however it clusters better.

martincostello · 2023-01-19T13:51:36Z

As noted in the description, I'm happy to chunk this up into N commits/PRs (whatever works best for you), but doing all the changes and pushing up a draft as a starting point was the easiest way for me to do the majority of the work in the first instance. The easiest way to fix with it turned on was per-file, per-project.

github-actions · 2023-01-27T03:16:22Z

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

github-actions · 2023-02-03T03:17:03Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

martincostello · 2023-02-03T14:22:32Z

Have opened #950 to track doing these changes in a series of separate pull requests.

martincostello added 22 commits January 19, 2023 09:00

Set AnalysisLevel

571d644

Set `AnalysisLevel` to `latest-All`.

Fix analysis warnings

65e7e03

Fix (or suppress) code analysis warnings for `OpenTelemetry.Contrib.Extensions.AWSXRay`.

Fix analysis warnings

b685a9e

Fix (or suppress) code analysis warnings for `OpenTelemetry.Contrib.Instrumentation.AWS`.

Fix analysis warnings

7b137de

Fix (or suppress) code analysis warnings for `OpenTelemetry.Contrib.Shared`.

Fix tests

5b750ab

Fix tests broken by 65e7e03.

Fix analysis warnings

ffdce3e

Fix (or suppress) code analysis warnings for `OpenTelemetry.Exporter.Instana`.

Fix analysis warnings

c41c8a2

Fix (or suppress) code analysis warnings for `OpenTelemetry.Exporter.Stackdriver`.

Fix analysis warnings

5c6e4ee

Fix (or suppress) code analysis warnings for `OpenTelemetry.Extensions.Docker`.

Fix analysis warnings

4fb51b3

ix (or suppress) code analysis warnings for `OpenTelemetry.Extensions.PersistentStorage`.

Fix analysis warnings

4319912

Fix (or suppress) code analysis warnings for `OpenTelemetry.Instrumentation.AspNet`.

Fix analysis warnings

72bf04c

Fix (or suppress) code analysis warnings for `OpenTelemetry.Instrumentation.AWSLambda`.

Fix analysis warnings

a836843

Fix (or suppress) code analysis warnings for `OpenTelemetry.Instrumentation.ElasticsearchClient`.

Fix analysis warning

4c85cb7

Fix (or suppress) code analysis warnings for `OpenTelemetry.Instrumentation.EntityFrameworkCore`.

Fix analysis warnings

5abada7

Fix (or suppress) code analysis warnings for `OpenTelemetry.Instrumentation.GrpcCore`.

Fix analysis warnings

f1b775d

Fix (or suppress) code analysis warnings for `OpenTelemetry.Contrib.Extensions.AWSXRay`.

Fix analysis warnings

7d373f6

Fix (or suppress) code analysis warnings for `OpenTelemetry.Instrumentation.MySqlData`.

Fix analysis warnings

6422104

Fix (or suppress) code analysis warnings for `OpenTelemetry.Contrib.Extensions.AWSXRay`.

Fix analysis warnings

7dbfcc0

Fix (or suppress) code analysis warnings for `OpenTelemetry.Instrumentation.Wcf`.

Use Guard.ThrowIfNull()

f977a7c

Use `Guard.ThrowIfNull()` where possible.

Fix analysis warnings

24a34c5

- Fix analysis warnings in non-production projects, such as tests and examples. - Fix analysis warnings in production projects missed in the first pass.

Fix CA1725 warning

5c02cba

Fix CA1725 warning by adjusting the public API.

Fix test

1c7b54f

Fix test broken by refactor to use `TryGetValue()`.

github-actions bot assigned cijothomas, CodeBlanch, hananiel, mic-max, Oberon00, reyang, SergeyKanzhelev and srprash Jan 19, 2023

github-actions bot assigned utpilla and vishweshbankwar Jan 19, 2023

martincostello mentioned this pull request Jan 19, 2023

Use Array.Empty<T>() #900

Merged

2 tasks

martincostello mentioned this pull request Jan 19, 2023

Do not assign people to draft pull requests #909

Merged

Oberon00 reviewed Jan 19, 2023

View reviewed changes

github-actions bot added the Stale label Jan 27, 2023

github-actions bot closed this Feb 3, 2023

martincostello mentioned this pull request Feb 3, 2023

Enable AnalysisLevel=latest-All and fix resulting issues open-telemetry/opentelemetry-dotnet#4144

Closed

martincostello mentioned this pull request Feb 3, 2023

Enable AnalysisLevel=latest-All and fix resulting issues #950

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable analysers #908

Enable analysers #908

martincostello commented Jan 19, 2023

codecov bot commented Jan 19, 2023

Oberon00 Jan 19, 2023

Kielek Jan 19, 2023

Oberon00 Jan 19, 2023

Oberon00 Jan 19, 2023

Oberon00 commented Jan 19, 2023

Kielek commented Jan 19, 2023

Oberon00 commented Jan 19, 2023

martincostello commented Jan 19, 2023

github-actions bot commented Jan 27, 2023

github-actions bot commented Feb 3, 2023

martincostello commented Feb 3, 2023 •

edited

Loading

Enable analysers #908

Enable analysers #908

Conversation

martincostello commented Jan 19, 2023

Changes

codecov bot commented Jan 19, 2023

Codecov Report

Oberon00 Jan 19, 2023

Choose a reason for hiding this comment

Kielek Jan 19, 2023

Choose a reason for hiding this comment

Oberon00 Jan 19, 2023

Choose a reason for hiding this comment

Oberon00 Jan 19, 2023

Choose a reason for hiding this comment

Oberon00 commented Jan 19, 2023

Kielek commented Jan 19, 2023

Oberon00 commented Jan 19, 2023

martincostello commented Jan 19, 2023

github-actions bot commented Jan 27, 2023

github-actions bot commented Feb 3, 2023

martincostello commented Feb 3, 2023 • edited Loading

martincostello commented Feb 3, 2023 •

edited

Loading