Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates for .NET 8 code analysis - Part 1 #1443

Conversation

utpilla
Copy link
Contributor

@utpilla utpilla commented Nov 22, 2023

Towards #1437

Changes

  • Fix the newer code analysis warnings from .NET 8 SDK
  • Update OneCollector project to add reference to System.Net.Http
  • Here a few common ones:
    • CA1859 Use concrete types when possible for improved performance
    • CA1309 CA1309: Use ordinal StringComparison
    • CA1513 Use ObjectDisposedException throw helper
    • CA1847 Use string.Contains(char) instead of string.Contains(string) with single characters

Copy link

codecov bot commented Nov 22, 2023

Codecov Report

Merging #1443 (86def0e) into main (71655ce) will decrease coverage by 0.24%.
Report is 54 commits behind head on main.
The diff coverage is 68.27%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1443      +/-   ##
==========================================
- Coverage   73.91%   73.67%   -0.24%     
==========================================
  Files         267      261       -6     
  Lines        9615     9687      +72     
==========================================
+ Hits         7107     7137      +30     
- Misses       2508     2550      +42     
Flag Coverage Δ
unittests-Exporter.Geneva 57.92% <31.48%> (?)
unittests-Exporter.OneCollector 89.72% <100.00%> (?)
unittests-Extensions 81.73% <ø> (?)
unittests-Instrumentation.AspNet 72.56% <94.44%> (?)
unittests-Instrumentation.EventCounters 75.92% <ø> (?)
unittests-Instrumentation.Owin 85.71% <100.00%> (?)
unittests-Instrumentation.Process 100.00% <ø> (?)
unittests-Instrumentation.Runtime 100.00% <ø> (?)
unittests-Instrumentation.StackExchangeRedis 71.37% <6.45%> (?)
unittests-Instrumentation.Wcf 78.47% <81.59%> (?)
unittests-PersistentStorage 58.80% <ø> (?)
unittests-ResourceDetectors.Azure 80.90% <60.00%> (?)
unittests-Solution 81.05% <75.92%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...xporter.Geneva/Internal/ConnectionStringBuilder.cs 92.94% <100.00%> (ø)
...r.Geneva/Metrics/GenevaMetricExporterExtensions.cs 93.33% <ø> (ø)
...orter.Geneva/MsgPackExporter/MsgPackLogExporter.cs 95.10% <100.00%> (+1.76%) ⬆️
...ter.Geneva/MsgPackExporter/MsgPackTraceExporter.cs 94.52% <100.00%> (+2.10%) ⬆️
...ry.Exporter.InfluxDB/InfluxDBExporterExtensions.cs 100.00% <100.00%> (ø)
....Exporter.OneCollector/Internal/CallbackManager.cs 96.00% <100.00%> (+0.16%) ⬆️
...ector/Internal/Transports/HttpJsonPostTransport.cs 95.41% <ø> (ø)
...ctor/Logs/OneCollectorLogExportProcessorBuilder.cs 67.74% <ø> (ø)
...ions.Enrichment/Internal/TraceEnrichmentActions.cs 100.00% <100.00%> (ø)
...ns.Enrichment/Internal/TraceEnrichmentProcessor.cs 100.00% <100.00%> (ø)
... and 59 more

... and 34 files with indirect coverage changes

Comment on lines +36 to +38
#if NET7_0_OR_GREATER
ObjectDisposedException.ThrowIf(this.disposed, nameof(CallbackManager<T>));
#else
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now we have only one place, so it is fine. Consider to extend Guard class with implementation for ThrowIfObjectDisposedExcpetion(bool disposed, Type type)

Comment on lines +83 to +87
#if NET6_0_OR_GREATER
int idx = websiteOwnerName.IndexOf('+', StringComparison.Ordinal);
#else
int idx = websiteOwnerName.IndexOf("+", StringComparison.Ordinal);
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From current implementation:

public int IndexOf(char value, StringComparison comparisonType)
{
    switch (comparisonType)
    {
        case StringComparison.CurrentCulture:
        case StringComparison.CurrentCultureIgnoreCase:
            return CultureInfo.CurrentCulture.CompareInfo.IndexOf(this, value, GetCaseCompareOfComparisonCulture(comparisonType));

        case StringComparison.InvariantCulture:
        case StringComparison.InvariantCultureIgnoreCase:
            return CompareInfo.Invariant.IndexOf(this, value, GetCaseCompareOfComparisonCulture(comparisonType));

        case StringComparison.Ordinal:
            return IndexOf(value);

        case StringComparison.OrdinalIgnoreCase:
            return CompareInfo.Invariant.IndexOf(this, value, CompareOptions.OrdinalIgnoreCase);

        default:
            throw new ArgumentException(SR.NotSupported_StringComparison, nameof(comparisonType));
    }

It should allow remove conditional compilation

Suggested change
#if NET6_0_OR_GREATER
int idx = websiteOwnerName.IndexOf('+', StringComparison.Ordinal);
#else
int idx = websiteOwnerName.IndexOf("+", StringComparison.Ordinal);
#endif
int idx = websiteOwnerName.IndexOf('+');

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to not rely on the implementation details here. It would be fine to use IndexOf('+') today but .NET team is free to change the implementation in .NET 9 or later.

@@ -202,7 +202,9 @@ internal BaseProcessor<LogRecord> BuildProcessor()

private OneCollectorExporter<LogRecord> BuildExporter()
{
#pragma warning disable CA2000 // Dispose objects before losing scope
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: You could try renaming this method "CreateExporter" it may resolve CA2000. It tries to use names of things to detect factory patterns where it is expected that something will be returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that but it didn't help.

Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@utpilla utpilla merged commit 5167fbe into open-telemetry:main Nov 22, 2023
66 of 67 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants