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

[HttpClient] Remove SDK dependency #5077

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,7 @@
using System.Net.Http;
#endif
using System.Runtime.CompilerServices;
using Microsoft.Extensions.Configuration;
using OpenTelemetry.Instrumentation.Http.Implementation;
using static OpenTelemetry.Internal.HttpSemanticConventionHelper;

namespace OpenTelemetry.Instrumentation.Http;

Expand All @@ -31,23 +29,6 @@ namespace OpenTelemetry.Instrumentation.Http;
/// </summary>
public class HttpClientInstrumentationOptions
{
internal readonly HttpSemanticConvention HttpSemanticConvention;

/// <summary>
/// Initializes a new instance of the <see cref="HttpClientInstrumentationOptions"/> class.
/// </summary>
public HttpClientInstrumentationOptions()
: this(new ConfigurationBuilder().AddEnvironmentVariables().Build())
{
}

internal HttpClientInstrumentationOptions(IConfiguration configuration)
{
Debug.Assert(configuration != null, "configuration was null");

this.HttpSemanticConvention = GetSemanticConventionOptIn(configuration);
}

/// <summary>
/// Gets or sets a filter function that determines whether or not to
/// collect telemetry on a per request basis.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,11 +104,6 @@ public void OnStartActivity(Activity activity, object payload)
// By this time, samplers have already run and
// activity.IsAllDataRequested populated accordingly.

if (Sdk.SuppressInstrumentation)
{
return;
}

if (!TryFetchRequest(payload, out HttpRequestMessage request))
{
HttpInstrumentationEventSource.Log.NullPayload(nameof(HttpHandlerDiagnosticListener), nameof(this.OnStartActivity));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,6 @@ public override void OnEventWritten(string name, object payload)

public void OnStopEventWritten(Activity activity, object payload)
{
if (Sdk.SuppressInstrumentation)
{
return;
}

if (TryFetchRequest(payload, out HttpRequestMessage request))
{
// see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-metrics.md
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,24 +12,22 @@

<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" />
<Compile Include="$(RepoRoot)\src\Shared\RequestMethodHelper.cs" Link="Includes\RequestMethodHelper.cs" />
</ItemGroup>

<ItemGroup Condition="'$(RunningDotNetPack)' != 'true'">
<ProjectReference Include="$(RepoRoot)\src\OpenTelemetry\OpenTelemetry.csproj" />
<ProjectReference Include="$(RepoRoot)\src\OpenTelemetry.Api.ProviderBuilderExtensions\OpenTelemetry.Api.ProviderBuilderExtensions.csproj" />
</ItemGroup>

<!-- Instrumentation packages when published should take a dependency only on the latest stable version of the API/SDK. -->
<ItemGroup Condition="'$(RunningDotNetPack)' == 'true'">
<PackageReference Include="OpenTelemetry" />
<PackageReference Include="OpenTelemetry.Api.ProviderBuilderExtensions" />
</ItemGroup>

<ItemGroup>
<Reference Include="System.Net.Http" Condition="'$(TargetFramework)' == '$(NetFrameworkMinimumSupportedVersion)'" />
<PackageReference Include="Microsoft.Extensions.Options" />
</ItemGroup>

</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,6 @@ public static TracerProviderBuilder AddHttpClientInstrumentation(
{
services.Configure(name, configureHttpClientInstrumentationOptions);
}

services.RegisterOptionsFactory(configuration => new HttpClientInstrumentationOptions(configuration));
});

#if NETFRAMEWORK
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ public void GrpcAndHttpClientInstrumentationIsInvoked(bool shouldEnrich)
Assert.Equal(shouldEnrich ? "yes" : "no", grpcSpan.Tags.Where(tag => tag.Key == "enrichedWithHttpResponseMessage").FirstOrDefault().Value);
}

[Fact]
[Fact(Skip = "Removed for stable release of http instrumentation")]
public void GrpcAndHttpClientInstrumentationWithSuppressInstrumentation()
{
var uri = new Uri($"http://localhost:{this.server.Port}");
Expand Down Expand Up @@ -443,7 +443,7 @@ public void GrpcAndHttpClientInstrumentationWithSuppressInstrumentation()
Assert.Equal(0, grpcSpan4.GetTagValue(SemanticConventions.AttributeRpcGrpcStatusCode));
}

[Fact]
[Fact(Skip = "Removed for stable release of http instrumentation")]
public void GrpcPropagatesContextWithSuppressInstrumentationOptionSetToTrue()
{
try
Expand Down Expand Up @@ -586,7 +586,7 @@ public void GrpcDoesNotPropagateContextWithSuppressInstrumentationOptionSetToFal
}
}

[Fact]
[Fact(Skip = "Removed for stable release of http instrumentation")]
public void GrpcClientInstrumentationRespectsSdkSuppressInstrumentation()
{
try
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ public async Task InjectsHeadersAsync_CustomFormat()
}));
}

[Fact]
[Fact(Skip = "Removed for stable release of http instrumentation")]
Copy link
Member

Choose a reason for hiding this comment

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

lets add a link to a tracking gh issue.

Copy link
Member Author

@vishweshbankwar vishweshbankwar Nov 28, 2023

Choose a reason for hiding this comment

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

Yes, that is my TODO on PR desc :D. Wanted to confirm everyone agrees with this proposal.

Copy link
Member

Choose a reason for hiding this comment

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

Also add in PR desc and/or changelog the full impact of this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

public async Task RespectsSuppress()
{
try
Expand Down
Loading