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.

Sign up for GitHub

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

Jump to bottom

[Traces] Support named options in HttpClient instrumentation #3664

Merged
merged 4 commits into from
Sep 15, 2022
Merged

[Traces] Support named options in HttpClient instrumentation #3664

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0988a82
Enabled named options in httpclient instrumentation.
CodeBlanch Sep 14, 2022
c13c462
Tests.
CodeBlanch Sep 14, 2022
16e8b82
CHANGELOG update.
CodeBlanch Sep 14, 2022
c1ed5f3
Merge branch 'main' into httpclient-namedoptions
cijothomas Sep 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
0 ...cApi/netstandard2.0/PublicAPI.Shipped.txt → ...p/.publicApi/net6.0/PublicAPI.Shipped.txt
View file
Open in desktop
File renamed without changes.
4 changes: 3 additions & 1 deletion ...pi/netstandard2.0/PublicAPI.Unshipped.txt → ....publicApi/net6.0/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,6 @@ OpenTelemetry.Instrumentation.Http.HttpClientInstrumentationOptions.RecordExcept
OpenTelemetry.Metrics.MeterProviderBuilderExtensions
OpenTelemetry.Trace.TracerProviderBuilderExtensions
static OpenTelemetry.Metrics.MeterProviderBuilderExtensions.AddHttpClientInstrumentation(this OpenTelemetry.Metrics.MeterProviderBuilder builder) -> OpenTelemetry.Metrics.MeterProviderBuilder
static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddHttpClientInstrumentation(this OpenTelemetry.Trace.TracerProviderBuilder builder, System.Action<OpenTelemetry.Instrumentation.Http.HttpClientInstrumentationOptions> configureHttpClientInstrumentationOptions = null) -> OpenTelemetry.Trace.TracerProviderBuilder
static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddHttpClientInstrumentation(this OpenTelemetry.Trace.TracerProviderBuilder builder) -> OpenTelemetry.Trace.TracerProviderBuilder
static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddHttpClientInstrumentation(this OpenTelemetry.Trace.TracerProviderBuilder builder, string name, System.Action<OpenTelemetry.Instrumentation.Http.HttpClientInstrumentationOptions> configureHttpClientInstrumentationOptions) -> OpenTelemetry.Trace.TracerProviderBuilder
static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddHttpClientInstrumentation(this OpenTelemetry.Trace.TracerProviderBuilder builder, System.Action<OpenTelemetry.Instrumentation.Http.HttpClientInstrumentationOptions> configureHttpClientInstrumentationOptions) -> OpenTelemetry.Trace.TracerProviderBuilder
8 changes: 8 additions & 0 deletions src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@

## Unreleased

* Dropped `netstandard2.0` target and added `net6.0`
([#3664](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3664))

* Added overloads which accept a name to the `TracerProviderBuilder`
`AddHttpClientInstrumentation` extension to allow for more fine-grained
options management
([#3664](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3664))

## 1.0.0-rc9.6

Released 2022-Aug-18
Expand Down
7 changes: 6 additions & 1 deletion src/OpenTelemetry.Instrumentation.Http/OpenTelemetry.Instrumentation.Http.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,17 @@

<PropertyGroup>
<!-- OmniSharp/VS Code requires TargetFrameworks to be in descending order for IntelliSense and analysis. -->
<TargetFrameworks>netstandard2.0;net462</TargetFrameworks>
<TargetFrameworks>net6.0;net462</TargetFrameworks>
Copy link
Member Author

Choose a reason for hiding this comment

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

@alanwest @utpilla @cijothomas Hey I dropped netstandard2.0 and added net6.0. The netstandard2.0 didn't really make sense to me. We've seen users reference this in netstandard2.0 projects which then blow up when consumed into net462 projects because the API is different. Seems like this is a safer way to go about things?

Copy link
Member

Choose a reason for hiding this comment

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

This would resolve #3434. Technically, this is sort of a breaking change.

I suspect that the use of the HttpClient instrumentation is pretty ubiquitous. Given that this change will require .NET Framework users who have shared netstandard libraries to retarget their libraries, I think we may as well rip the bandaid off and remove netstandard targets from all our packages.

Copy link
Member

Choose a reason for hiding this comment

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

agree with this change to HttpClientInstrumenttion.
Those consuming this via shared library will see breakage, unless the shared library also updates their target.
Those consuming this directly from app (like a Asp.Net or Asp.Net core) app should see no breakage.

Changing ns20 target from API/SDK packages should be discussed separately, as those are stable packages, unlike httpclient instrumentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I merged this. Felt like it was good to get it in, get it out there, see if we get any feedback.

This change would have also worked:

<TargetFrameworks>netstandard2.1;net462</TargetFrameworks>

Because .NET Framework does not implement .NET Standard 2.1, users would get the correct version in that case.

Not sure which is the best way to go.

  • We don't have netstandard2.1 in AspNetCore instrumentation currently.
  • We do have netstandard2.0 in API+SDK and netstandard2.1 in SDK.

If our goal is to drop support for <net6.0 we should maybe just go to <TargetFrameworks>net6.0;net462</TargetFrameworks> across the board. It will be bumpy, but clear. Otherwise perhaps we should keep netstandard2.1 in HttpClient + AspNetCore.

<Description>Http instrumentation for OpenTelemetry .NET</Description>
<PackageTags>$(PackageTags);distributed-tracing</PackageTags>
<IncludeDiagnosticSourceInstrumentationHelpers>true</IncludeDiagnosticSourceInstrumentationHelpers>
</PropertyGroup>

<!--Do not run ApiCompat for net6.0 as this is newly added. Remove this property once we have released a stable version.-->
<PropertyGroup Condition="'$(TargetFramework)' == 'net6.0'">
<RunApiCompat>false</RunApiCompat>
</PropertyGroup>

<ItemGroup>
<Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\Guard.cs" Link="Includes\Guard.cs" />
</ItemGroup>
Expand Down
31 changes: 27 additions & 4 deletions src/OpenTelemetry.Instrumentation.Http/TracerProviderBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,22 +61,45 @@ public static TracerProviderBuilder AddHttpClientInstrumentation(
/// Enables HttpClient instrumentation.
/// </summary>
/// <param name="builder"><see cref="TracerProviderBuilder"/> being configured.</param>
/// <param name="configureHttpClientInstrumentationOptions">HttpClient configuration options.</param>
/// <returns>The instance of <see cref="TracerProviderBuilder"/> to chain the calls.</returns>
public static TracerProviderBuilder AddHttpClientInstrumentation(this TracerProviderBuilder builder)
=> AddHttpClientInstrumentation(builder, name: null, configureHttpClientInstrumentationOptions: null);

/// <summary>
/// Enables HttpClient instrumentation.
/// </summary>
/// <param name="builder"><see cref="TracerProviderBuilder"/> being configured.</param>
/// <param name="configureHttpClientInstrumentationOptions">Callback action for configuring <see cref="HttpClientInstrumentationOptions"/>.</param>
/// <returns>The instance of <see cref="TracerProviderBuilder"/> to chain the calls.</returns>
public static TracerProviderBuilder AddHttpClientInstrumentation(
this TracerProviderBuilder builder,
Action<HttpClientInstrumentationOptions> configureHttpClientInstrumentationOptions = null)
Action<HttpClientInstrumentationOptions> configureHttpClientInstrumentationOptions)
=> AddHttpClientInstrumentation(builder, name: null, configureHttpClientInstrumentationOptions);

/// <summary>
/// Enables HttpClient instrumentation.
/// </summary>
/// <param name="builder"><see cref="TracerProviderBuilder"/> being configured.</param>
/// <param name="name">Name which is used when retrieving options.</param>
/// <param name="configureHttpClientInstrumentationOptions">Callback action for configuring <see cref="HttpClientInstrumentationOptions"/>.</param>
/// <returns>The instance of <see cref="TracerProviderBuilder"/> to chain the calls.</returns>
public static TracerProviderBuilder AddHttpClientInstrumentation(
this TracerProviderBuilder builder,
string name,
Action<HttpClientInstrumentationOptions> configureHttpClientInstrumentationOptions)
{
Guard.ThrowIfNull(builder);

name ??= Options.DefaultName;

if (configureHttpClientInstrumentationOptions != null)
{
builder.ConfigureServices(services => services.Configure(configureHttpClientInstrumentationOptions));
builder.ConfigureServices(services => services.Configure(name, configureHttpClientInstrumentationOptions));
}

return builder.ConfigureBuilder((sp, builder) =>
{
var options = sp.GetRequiredService<IOptions<HttpClientInstrumentationOptions>>().Value;
var options = sp.GetRequiredService<IOptionsMonitor<HttpClientInstrumentationOptions>>().Get(name);

AddHttpClientInstrumentation(builder, new HttpClientInstrumentation(options));
});
Expand Down
22 changes: 22 additions & 0 deletions test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
using System.Linq;
using System.Net.Http;
using System.Threading.Tasks;
using Microsoft.Extensions.DependencyInjection;
using Moq;
using OpenTelemetry.Context.Propagation;
using OpenTelemetry.Instrumentation.Http.Implementation;
Expand Down Expand Up @@ -47,6 +48,27 @@ public HttpClientTests()
this.url = $"http://{host}:{port}/";
}

[Fact]
public void AddHttpClientInstrumentation_NamedOptions()
{
int defaultExporterOptionsConfigureOptionsInvocations = 0;
int namedExporterOptionsConfigureOptionsInvocations = 0;

using var tracerProvider = Sdk.CreateTracerProviderBuilder()
.ConfigureServices(services =>
{
services.Configure<HttpClientInstrumentationOptions>(o => defaultExporterOptionsConfigureOptionsInvocations++);

services.Configure<HttpClientInstrumentationOptions>("Instrumentation2", o => namedExporterOptionsConfigureOptionsInvocations++);
})
.AddHttpClientInstrumentation()
.AddHttpClientInstrumentation("Instrumentation2", configureHttpClientInstrumentationOptions: null)
.Build();

Assert.Equal(1, defaultExporterOptionsConfigureOptionsInvocations);
Assert.Equal(1, namedExporterOptionsConfigureOptionsInvocations);
}

[Fact]
public void AddHttpClientInstrumentation_BadArgs()
{
Expand Down