Merge branch 'main' into instrumentation-grpcnetclient-rename-options…

…-class
open-telemetry · Jan 30, 2024 · 1440b22 · 1440b22
2 parents 1a1a473 + edc9fe2
commit 1440b22
Show file tree

Hide file tree

Showing 24 changed files with 264 additions and 405 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -32,12 +32,16 @@ jobs:
 
   lint-md:
     needs: detect-changes
-    if: contains(needs.detect-changes.outputs.changes, 'md')
+    if: |
+      contains(needs.detect-changes.outputs.changes, 'md')
+      || contains(needs.detect-changes.outputs.changes, 'build')
     uses: ./.github/workflows/markdownlint.yml
 
   lint-dotnet-format:
     needs: detect-changes
-    if: contains(needs.detect-changes.outputs.changes, 'code')
+    if: |
+      contains(needs.detect-changes.outputs.changes, 'code')
+      || contains(needs.detect-changes.outputs.changes, 'build')
     uses: ./.github/workflows/dotnet-format.yml
 
   build-test-solution-stable:

diff --git a/.github/workflows/markdownlint.yml b/.github/workflows/markdownlint.yml
@@ -13,8 +13,9 @@ jobs:
     - name: check out code
       uses: actions/checkout@v4
 
-    - name: install markdownlint-cli
-      run: sudo npm install -g markdownlint-cli
-
     - name: run markdownlint
-      run: markdownlint .
+      uses: DavidAnson/[email protected]
+      with:
+        globs: |
+          **/*.md
+          !.github/**/*.md
diff --git a/OpenTelemetry.sln b/OpenTelemetry.sln
@@ -265,7 +265,6 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Shared", "Shared", "{A49299
 		src\Shared\DiagnosticDefinitions.cs = src\Shared\DiagnosticDefinitions.cs
 		src\Shared\ExceptionExtensions.cs = src\Shared\ExceptionExtensions.cs
 		src\Shared\Guard.cs = src\Shared\Guard.cs
-		src\Shared\HttpSemanticConventionHelper.cs = src\Shared\HttpSemanticConventionHelper.cs
 		src\Shared\MathHelper.cs = src\Shared\MathHelper.cs
 		src\Shared\PeerServiceResolver.cs = src\Shared\PeerServiceResolver.cs
 		src\Shared\PeriodicExportingMetricReaderHelper.cs = src\Shared\PeriodicExportingMetricReaderHelper.cs

diff --git a/docs/logs/README.md b/docs/logs/README.md
@@ -5,9 +5,9 @@
 The following tutorials have demonstrated the best practices for logging with
 OpenTelemetry .NET:
 
-* [Getting Started - Console Application](./getting-started-console/README.md)
 * [Getting Started - ASP.NET Core
   Application](./getting-started-aspnetcore/README.md)
+* [Getting Started - Console Application](./getting-started-console/README.md)
 * [Logging with Complex Objects](./complex-objects/README.md)
 
 ## Structured Logging

diff --git a/docs/metrics/README.md b/docs/metrics/README.md
@@ -2,35 +2,75 @@
 
 ## Best Practices
 
-### Instruments should be singleton
-
-Instruments SHOULD only be created once and reused throughout the application
-lifetime. This [example](../../docs/metrics/getting-started-console/Program.cs)
-shows how an instrument is created as a `static` field and then used in the
-application. You could also look at this ASP.NET Core
-[example](../../examples/AspNetCore/Program.cs) which shows a more Dependency
-Injection friendly way of doing this by extracting the `Meter` and an instrument
-into a dedicated class called
-[Instrumentation](../../examples/AspNetCore/Instrumentation.cs) which is then
-added as a `Singleton` service.
-
-### Ordering of tags
-
-When emitting metrics with tags, DO NOT change the order in which you provide
-tags. Changing the order of tag keys would increase the time taken by the SDK to
-record the measurement.
+The following tutorials have demonstrated the best practices for while using
+metrics with OpenTelemetry .NET:
+
+* [Getting Started - ASP.NET Core
+  Application](./getting-started-aspnetcore/README.md)
+* [Getting Started - Console Application](./getting-started-console/README.md)
+
+## Package Version
+
+:heavy_check_mark: You should always use the
+[System.Diagnostics.Metrics](https://learn.microsoft.com/dotnet/api/system.diagnostics.metrics)
+APIs from the latest stable version of
+[System.Diagnostics.DiagnosticSource](https://www.nuget.org/packages/System.Diagnostics.DiagnosticSource/)
+package, regardless of the .NET runtime version being used:
+
+* If you're using the latest stable version of [OpenTelemetry .NET
+  SDK](../../src/OpenTelemetry/README.md), you don't have to worry about the
+  version of `System.Diagnostics.DiagnosticSource` package because it is already
+  taken care of for you via [package
+  dependency](../../Directory.Packages.props).
+* The .NET runtime team is holding a high bar for backward compatibility on
+  `System.Diagnostics.DiagnosticSource` even during major version bumps, so
+  compatibility is not a concern here.
+
+## Metrics API
+
+:heavy_check_mark: You should understand and pick the right instrument type.
+
+  > [!NOTE]
+  > .NET runtime has provided several instrument types based on the [OpenTelemetry
+    Specification](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument).
+    Picking the right instrument type for your use case is crucial to ensure the
+    correct semantics and performance. Check the [Instrument
+    Selection](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/supplementary-guidelines.md#instrument-selection)
+    section from the supplementary guidelines for more information.
+
+  | OpenTelemetry Specification | .NET Instrument Type |
+  | --------------------------- | -------------------- |
+  | [Asynchronous Counter](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#asynchronous-counter) | [`ObservableCounter<T>`](https://learn.microsoft.com/dotnet/api/system.diagnostics.metrics.observablecounter-1) |
+  | [Asynchronous Gauge](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#asynchronous-gauge) | [`ObservableGauge<T>`](https://learn.microsoft.com/dotnet/api/system.diagnostics.metrics.observablegauge-1) |
+  | [Asynchronous UpDownCounter](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#asynchronous-updowncounter) | [`ObservableUpDownCounter<T>`](https://learn.microsoft.com/dotnet/api/system.diagnostics.metrics.observableupdowncounter-1) |
+  | [Counter](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#counter) | [`Counter<T>`](https://learn.microsoft.com/dotnet/api/system.diagnostics.metrics.counter-1) |
+  | [Gauge](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#gauge) (experimental) | N/A |
+  | [Histogram](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#histogram) | [`Histogram<T>`](https://learn.microsoft.com/dotnet/api/system.diagnostics.metrics.histogram-1) |
+  | [UpDownCounter](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#updowncounter) | [`UpDownCounter<T>`](https://learn.microsoft.com/dotnet/api/system.diagnostics.metrics.updowncounter-1) |
+
+:stop_sign: You should avoid creating instruments (e.g. `Counter<T>`) too
+frequently. Instruments are fairly expensive and meant to be reused throughout
+the application. For most applications, instruments can be modeled as static
+readonly fields (e.g. [Program.cs](./getting-started-console/Program.cs)) or
+singleton via dependency injection (e.g.
+[Instrumentation.cs](../../examples/AspNetCore/Instrumentation.cs)).
+
+:stop_sign: You should avoid changing the order of tags while reporting
+measurements.
+
+> [!WARNING]
+> The last line of code has bad performance since the tags are not following
+  the same order:
 
 ```csharp
-// If you emit the tag keys in this order: name -> color -> taste, stick to this order of tag keys for subsequent measurements.
-MyFruitCounter.Add(5, new("name", "apple"), new("color", "red"), new("taste", "sweet"));
-...
-...
-...
-// Same measurement with the order of tags changed: color -> name -> taste. This order of tags is different from the one that was first encountered by the SDK.
-MyFruitCounter.Add(7, new("color", "red"), new("name", "apple"), new("taste", "sweet")); // <--- DON'T DO THIS
+counter.Add(2, new("name", "apple"), new("color", "red"));
+counter.Add(3, new("name", "lime"), new("color", "green"));
+counter.Add(5, new("name", "lemon"), new("color", "yellow"));
+counter.Add(8, new("color", "yellow"), new("name", "lemon")); // bad perf
 ```
 
-### Use TagList accordingly
+:heavy_check_mark: You should use TagList properly to achieve the best
+performance.
 
 There are two different ways of passing tags to an instrument API:
 

diff --git a/src/OpenTelemetry.Api/Context/Propagation/TraceContextPropagator.cs b/src/OpenTelemetry.Api/Context/Propagation/TraceContextPropagator.cs
@@ -113,8 +113,12 @@ public override void Inject<T>(PropagationContext context, T carrier, Action<T,
             return;
         }
 
+#if NET6_0_OR_GREATER
+        var traceparent = string.Create(55, context.ActivityContext, WriteTraceParentIntoSpan);
+#else
         var traceparent = string.Concat("00-", context.ActivityContext.TraceId.ToHexString(), "-", context.ActivityContext.SpanId.ToHexString());
         traceparent = string.Concat(traceparent, (context.ActivityContext.TraceFlags & ActivityTraceFlags.Recorded) != 0 ? "-01" : "-00");
+#endif
 
         setter(carrier, TraceParent, traceparent);
 
@@ -425,4 +429,22 @@ private static bool IsLowerAlphaDigit(char c)
     {
         return (c >= '0' && c <= '9') || (c >= 'a' && c <= 'z');
     }
+
+#if NET6_0_OR_GREATER
+    private static void WriteTraceParentIntoSpan(Span<char> destination, ActivityContext context)
+    {
+        "00-".CopyTo(destination);
+        context.TraceId.ToHexString().CopyTo(destination.Slice(3));
+        destination[35] = '-';
+        context.SpanId.ToHexString().CopyTo(destination.Slice(36));
+        if ((context.TraceFlags & ActivityTraceFlags.Recorded) != 0)
+        {
+            "-01".CopyTo(destination.Slice(52));
+        }
+        else
+        {
+            "-00".CopyTo(destination.Slice(52));
+        }
+    }
+#endif
 }
diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md
@@ -2,6 +2,13 @@
 
 ## Unreleased
 
+* Removed support for the `OTEL_SEMCONV_STABILITY_OPT_IN` environment variable
+  which toggled the use of the new conventions for the
+  [server, client, and shared network attributes](https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/general/attributes.md#server-client-and-shared-network-attributes).
+  Now that this suite of attributes are stable, this instrumentation will only
+  emit the new attributes.
+  ([#5270](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5270))
+
 ## 1.6.0-beta.3
 
 Released 2023-Nov-17

diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/OpenTelemetry.Instrumentation.SqlClient.csproj b/src/OpenTelemetry.Instrumentation.SqlClient/OpenTelemetry.Instrumentation.SqlClient.csproj
@@ -12,9 +12,6 @@
 
   <ItemGroup>
     <Compile Include="$(RepoRoot)\src\Shared\Guard.cs" Link="Includes\Guard.cs" />
-    <Compile Include="$(RepoRoot)\src\Shared\EnvironmentVariables\*.cs" Link="Includes\EnvironmentVariables\%(Filename).cs" />
-    <Compile Include="$(RepoRoot)\src\Shared\HttpSemanticConventionHelper.cs" Link="Includes\HttpSemanticConventionHelper.cs" />
-    <Compile Include="$(RepoRoot)\src\Shared\Options\*.cs" Link="Includes\Options\%(Filename).cs" />
     <Compile Include="$(RepoRoot)\src\Shared\Shims\NullableAttributes.cs" Link="Includes\Shims\NullableAttributes.cs" />
   </ItemGroup>
 
@@ -29,7 +26,6 @@
 
   <ItemGroup>
     <PackageReference Include="Microsoft.Extensions.Options" />
-    <PackageReference Include="Microsoft.Extensions.Configuration" />
   </ItemGroup>
 
 </Project>
diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/SqlClientInstrumentationOptions.cs b/src/OpenTelemetry.Instrumentation.SqlClient/SqlClientInstrumentationOptions.cs
@@ -5,9 +5,7 @@
 using System.Data;
 using System.Diagnostics;
 using System.Text.RegularExpressions;
-using Microsoft.Extensions.Configuration;
 using OpenTelemetry.Trace;
-using static OpenTelemetry.Internal.HttpSemanticConventionHelper;
 
 namespace OpenTelemetry.Instrumentation.SqlClient;
 
@@ -52,26 +50,6 @@ public class SqlClientInstrumentationOptions
 
     private static readonly ConcurrentDictionary<string, SqlConnectionDetails> ConnectionDetailCache = new(StringComparer.OrdinalIgnoreCase);
 
-    private readonly bool emitOldAttributes;
-    private readonly bool emitNewAttributes;
-
-    /// <summary>
-    /// Initializes a new instance of the <see cref="SqlClientInstrumentationOptions"/> class.
-    /// </summary>
-    public SqlClientInstrumentationOptions()
-        : this(new ConfigurationBuilder().AddEnvironmentVariables().Build())
-    {
-    }
-
-    internal SqlClientInstrumentationOptions(IConfiguration configuration)
-    {
-        Debug.Assert(configuration != null, "configuration was null");
-
-        var httpSemanticConvention = GetSemanticConventionOptIn(configuration);
-        this.emitOldAttributes = httpSemanticConvention.HasFlag(HttpSemanticConvention.Old);
-        this.emitNewAttributes = httpSemanticConvention.HasFlag(HttpSemanticConvention.New);
-    }
-
     /// <summary>
     /// Gets or sets a value indicating whether or not the <see
     /// cref="SqlClientInstrumentation"/> should add the names of <see
@@ -130,13 +108,6 @@ internal SqlClientInstrumentationOptions(IConfiguration configuration)
     /// <para>
     /// The default behavior is to set the SqlConnection DataSource as the <see cref="SemanticConventions.AttributePeerService"/> tag.
     /// If enabled, SqlConnection DataSource will be parsed and the server name will be sent as the
-    /// <see cref="SemanticConventions.AttributeNetPeerName"/> or <see cref="SemanticConventions.AttributeNetPeerIp"/> tag,
-    /// the instance name will be sent as the <see cref="SemanticConventions.AttributeDbMsSqlInstanceName"/> tag,
-    /// and the port will be sent as the <see cref="SemanticConventions.AttributeNetPeerPort"/> tag if it is not 1433 (the default port).
-    /// </para>
-    /// <para>
-    /// If the environment variable OTEL_SEMCONV_STABILITY_OPT_IN is set to "http", the newer Semantic Convention v1.21.0 Attributes will be emitted.
-    /// SqlConnection DataSource will be parsed and the server name will be sent as the
     /// <see cref="SemanticConventions.AttributeServerAddress"/> or <see cref="SemanticConventions.AttributeServerSocketAddress"/> tag,
     /// the instance name will be sent as the <see cref="SemanticConventions.AttributeDbMsSqlInstanceName"/> tag,
     /// and the port will be sent as the <see cref="SemanticConventions.AttributeServerPort"/> tag if it is not 1433 (the default port).
@@ -301,40 +272,19 @@ internal void AddConnectionLevelDetailsToActivity(string dataSource, Activity sq
                 sqlActivity.SetTag(SemanticConventions.AttributeDbMsSqlInstanceName, connectionDetails.InstanceName);
             }
 
-            if (this.emitOldAttributes)
+            if (!string.IsNullOrEmpty(connectionDetails.ServerHostName))
             {
-                if (!string.IsNullOrEmpty(connectionDetails.ServerHostName))
-                {
-                    sqlActivity.SetTag(SemanticConventions.AttributeNetPeerName, connectionDetails.ServerHostName);
-                }
-                else
-                {
-                    sqlActivity.SetTag(SemanticConventions.AttributeNetPeerIp, connectionDetails.ServerIpAddress);
-                }
-
-                if (!string.IsNullOrEmpty(connectionDetails.Port))
-                {
-                    sqlActivity.SetTag(SemanticConventions.AttributeNetPeerPort, connectionDetails.Port);
-                }
+                sqlActivity.SetTag(SemanticConventions.AttributeServerAddress, connectionDetails.ServerHostName);
             }
-
-            // see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/docs/database/database-spans.md
-            if (this.emitNewAttributes)
+            else
             {
-                if (!string.IsNullOrEmpty(connectionDetails.ServerHostName))
-                {
-                    sqlActivity.SetTag(SemanticConventions.AttributeServerAddress, connectionDetails.ServerHostName);
-                }
-                else
-                {
-                    sqlActivity.SetTag(SemanticConventions.AttributeServerSocketAddress, connectionDetails.ServerIpAddress);
-                }
+                sqlActivity.SetTag(SemanticConventions.AttributeServerSocketAddress, connectionDetails.ServerIpAddress);
+            }
 
-                if (!string.IsNullOrEmpty(connectionDetails.Port))
-                {
-                    // TODO: Should we continue to emit this if the default port (1433) is being used?
-                    sqlActivity.SetTag(SemanticConventions.AttributeServerPort, connectionDetails.Port);
-                }
+            if (!string.IsNullOrEmpty(connectionDetails.Port))
+            {
+                // TODO: Should we continue to emit this if the default port (1433) is being used?
+                sqlActivity.SetTag(SemanticConventions.AttributeServerPort, connectionDetails.Port);
             }
         }
     }

diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/TracerProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.SqlClient/TracerProviderBuilderExtensions.cs
@@ -62,15 +62,10 @@ public static TracerProviderBuilder AddSqlClientInstrumentation(
 
         name ??= Options.DefaultName;
 
-        builder.ConfigureServices(services =>
+        if (configureSqlClientInstrumentationOptions != null)
         {
-            if (configureSqlClientInstrumentationOptions != null)
-            {
-                services.Configure(name, configureSqlClientInstrumentationOptions);
-            }
-
-            services.RegisterOptionsFactory(configuration => new SqlClientInstrumentationOptions(configuration));
-        });
+            builder.ConfigureServices(services => services.Configure(name, configureSqlClientInstrumentationOptions));
+        }
 
         builder.AddInstrumentation(sp =>
         {

diff --git a/src/OpenTelemetry/Batch.cs b/src/OpenTelemetry/Batch.cs
@@ -71,7 +71,11 @@ public void Dispose()
                 T item = this.circularBuffer.Read();
                 if (typeof(T) == typeof(LogRecord))
                 {
-                    LogRecordSharedPool.Current.Return((LogRecord)(object)item);
+                    var logRecord = (LogRecord)(object)item;
+                    if (logRecord.Source == LogRecord.LogRecordSource.FromSharedPool)
+                    {
+                        LogRecordSharedPool.Current.Return(logRecord);
+                    }
                 }
             }
         }
@@ -134,7 +138,11 @@ public struct Enumerator : IEnumerator<T>
 
                 if (currentItem != null)
                 {
-                    LogRecordSharedPool.Current.Return((LogRecord)(object)currentItem);
+                    var logRecord = (LogRecord)(object)currentItem;
+                    if (logRecord.Source == LogRecord.LogRecordSource.FromSharedPool)
+                    {
+                        LogRecordSharedPool.Current.Return(logRecord);
+                    }
                 }
 
                 if (circularBuffer!.RemovedCount < enumerator.targetCount)
@@ -215,7 +223,12 @@ public void Dispose()
                 var currentItem = this.current;
                 if (currentItem != null)
                 {
-                    LogRecordSharedPool.Current.Return((LogRecord)(object)currentItem);
+                    var logRecord = (LogRecord)(object)currentItem;
+                    if (logRecord.Source == LogRecord.LogRecordSource.FromSharedPool)
+                    {
+                        LogRecordSharedPool.Current.Return(logRecord);
+                    }
+
                     this.current = null;
                 }
             }

diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md
@@ -13,6 +13,12 @@
   state for cumulative temporality.
   [#5230](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5230)
 
+* Fixed an issue causing `LogRecord`s to be incorrectly reused when wrapping an
+  instance of `BatchLogRecordExportProcessor` inside another
+  `BaseProcessor<LogRecord>` which leads to missing or incorrect data during
+  export.
+  [#5255](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5255)
+
 ## 1.7.0
 
 Released 2023-Dec-08