search

open-telemetry / opentelemetry-dotnet-contrib

This repository contains set of components extending functionality of the OpenTelemetry .NET SDK. Instrumentation libraries, exporters, and other components can find their home here.
https://opentelemetry.io
Apache License 2.0
445 stars 271 forks source link

Make DisableUrlQueryRedaction option in OpenTelemetry.Instrumentation.Http public #1954

Open ImoutoChan opened 1 month ago

ImoutoChan commented 1 month ago

Component

OpenTelemetry.Instrumentation.Http

Package Version

Package Name Version
OpenTelemetry.Api 1.9.0
OpenTelemetry 1.9.0

Runtime Version

net8.0

Description

After update URLs in traces became redacted. There is a property to disable it, but it's internal.

internal bool DisableUrlQueryRedaction { get; set; }

Why is this property internal? If I can disable it through environment variable, I should be able to disable it through code. Please make it public and set the default value however you like. You're forcing developers to use reflection, this is bad design decision.

You also hide the fact itself that it can be disabled, no one expect hidden internal property on options configuration object.

Steps to Reproduce

No response

Expected Result

No response

Actual Result

No response

Additional Context

The ugly but acceptable workaround is to disable access checks. Add it to your project file:

<ItemGroup>
    <PackageReference Include="IgnoresAccessChecksToGenerator" Version="0.7.0" PrivateAssets="All" />
    <InternalsAssemblyName Include="OpenTelemetry.Instrumentation.Http" />
    <InternalsAssemblyName Include="OpenTelemetry.Instrumentation.AspNetCore" />
</ItemGroup>

After that you'll be able to change these properties. Also the same problem is with AspNetCoreInstrumentation, don't forget to disable it there too.

It uses https://github.com/aelij/IgnoresAccessChecksToGenerator, give the author some love.

TimothyMothra commented 1 month ago

Hi @ImoutoChan The current feature is still Experimental which is why it's behind the OTEL_DOTNET_EXPERIMENTAL_HTTPCLIENT_DISABLE_URL_QUERY_REDACTION environment variable.

We're waiting for the OpenTelemetry Community to define how to handle this feature. That conversation is still ongoing: https://github.com/open-telemetry/semantic-conventions/pull/961

ImoutoChan commented 1 month ago

That conversation is still ongoing

I understand that some features are still experimetal, but then you should provide them as opt-in, not as opt-out. Right now this experimental feature affects all users who updated their packages to the latest non-preview version.

bmajik commented 1 month ago

As @ImoutoChan says, this could have been made trivial to opt-in OR opt-out, but that wasn't done.

This is a breaking change, with no semVer bump. Also exposing the option for people who use Builder and options function semantics would have been the right thing to do, especially since so many tutorials/docs use that convention for configuration.