Make DisableUrlQueryRedaction option in OpenTelemetry.Instrumentation.Http public

[ImoutoChan]

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]

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: open-telemetry/semantic-conventions#961

[ImoutoChan]

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]

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.

[Wraith2]

I have no problem with redaction existing in the current form and being enabled by default, but I do want to be able to deal with it in a natural way for the language/framework that I'm using. For .Net that means IConfiguration and options classes not only environment variables.

The fact that the feature is experimental is immaterial. It has changed the observable behaviour of the library. If it was intended to be hidden and only used by people choosing to use experimental features then it needed to be opt-in not opt-out.

If you wanted to you could use the new experimental attribute https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-12.0/experimental-attribute and ifdef obsolete's for down level tfm's forcing people to see and disable the warnings.

How long do we have to wait for the semantic convention changes to be approved or rejected? the issue has gone stale several times already.

[strue36]

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: open-telemetry/semantic-conventions#961

The linked PR was closed and replaced by one that is more selective. What further decisions by the OpenTelemetry Community are being waited on in order to determine how this redaction should be configured?