Instrumentation.AspNetCore and Instrumentation.Http 1.8.1 have illegal breaking changes in the produced telemetry

[pellared]

Bug Report

List of all OpenTelemetry NuGet packages and version that you are using (e.g. OpenTelemetry 1.0.2):

• https://github.com/open-telemetry/opentelemetry-dotnet/releases/tag/Instrumentation.AspNetCore-1.8.1
• https://github.com/open-telemetry/opentelemetry-dotnet/releases/tag/Instrumentation.Http-1.8.1

Symptom

The breaking changes introduced in #5532 are violating https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/telemetry-stability.md#fixed-schema-telemetry-producers.

Instrumentations that are labeled Stable and do not include the Schema URL in the produced telemetry are called Fixed Schema Telemetry Producers.

Such instrumentations are prohibited from changing any produced telemetry. If the specification changes over time and the semantic conventions are updated, the instrumentation is still prohibited from adopting the changes. If the instrumentation wishes to adopt the semantic convention changes it must first become a Schema-File Driven Telemetry Producer by adding an appropriate Schema URL in the produced telemetry.

I find such changes unacceptable as they can break analysis tools that consumes the instrumentation (e.g. dashboards, alerts, queries, etc.) especially that this changes is NOT required by the HTTP semantic conventions:

Sensitive content provided in url.full/url.query SHOULD be scrubbed when instrumentations can identify it.

Has this change been seen as acceptable by OTel Specification or/and OTel Semantic Conventions SIGs?

Notice that people are already complaining: #5532 (comment)

I think that such functionality MUST be changed to opt-in especially that it is not required by semantic conventions.

OTEL_DOTNET_EXPERIMENTAL_ASPNETCORE_DISABLE_URL_QUERY_REDACTION and OTEL_DOTNET_EXPERIMENTAL_HTTPCLIENT_DISABLE_URL_QUERY_REDACTION should be true by default.

[pellared]

CC @open-telemetry/semconv-http-approvers @open-telemetry/specs-semconv-approvers

[pellared]

I also want to mention that it is related to the issue below:

• Semantic conventions vs GDPR semantic-conventions#128
• Guidelines for redacting sensitive information semantic-conventions#877

Personally, I always advocated for not emitting attributes that have non-low probability to contain sensitive information. But the ship has sailed.

[lmolkova]

Sensitive content provided in url.full/url.query SHOULD be scrubbed when instrumentations can identify it.

Has this change been seen as acceptable by OTel Specification or/and OTel Semantic Conventions SIGs?

I believe there is a discussion here open-telemetry/semantic-conventions#860 (among other places).
The spec is somewhat vague wrt scrubbing and leaves a room for instrumentations to decide if/how they want to do it.

If ASP.NET Core was instrumented natively, I think it'd follow the logging defaults where query params are not logged by default.

I don't see a strict spec violation in the .NET instrumentation behavior (scrubbing is recommended if it can be identified). And I can understand the motivation to consider this change as a security hotfix. But I can also see that we need to be more specific in the spec wrt opt-in/opt-out behavior.

@pellared could you create an issue in the semconv repo? Also, it would be awesome if you could attend SemConv SIG meeting on Monday at 8am PST to bring it up.

[pellared]

I don't see a strict spec violation in the .NET instrumentation behavior

It violates https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/telemetry-stability.md#fixed-schema-telemetry-producers as since https://github.com/open-telemetry/opentelemetry-dotnet/releases/tag/Instrumentation.AspNetCore-1.6.0 it even promised to produce stable telemetry

The change is introduced in order to support stable release of http instrumentation.

@pellared could you create an issue in the semconv repo?

I am not sure what issue I should create. Feel free to create one. I found already 3 which are related:

• Semantic conventions vs GDPR semantic-conventions#128
• Http client and server span default collection behavior for url.full and url.query attributes semantic-conventions#860
• Guidelines for redacting sensitive information semantic-conventions#877

Also, it would be awesome if you could attend SemConv SIG meeting on Monday at 8am PST to bring it up.

I do my best to join but no promises.

[cijothomas]

It violates https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/telemetry-stability.md#fixed-schema-telemetry-producers

Not an answer to the original concern you have raised, but just want to highlight that the above spec is an experimental one.

[pellared]

just want to highlight that the above spec is an experimental one.

During the Specification SIG meeting (even last one) it was said that it an "aspiring" one. I am not sure why it is not stable yet.

EDIT: I just created:

• Stabilize Telemetry Stability document opentelemetry-specification#3998

[julealgon]

I already voiced my opinion in the initial PR where this redaction logic was introduced but will share here as well: I think performing this redaction by default is bad. The framework already defines a standard redaction library that could be leveraged for this that is opt-in, and the collector also has redaction capabilities that are similarly opt-in.

Sensitive content provided in url.full/url.query SHOULD be scrubbed when instrumentations can identify it.

This section quoted by @pellared seems quite clear to me as well. The bolded part being the most important for me. The answer to that, today, is "no" IMHO: the instrumentation cannot identify whether those should be scrubbed, all we are doing is a blanked redaction that just scrubs everything, that to me is not really the same. There is no such thing as "sensitive content detection" going on in the URL and Query components at all.

I was also opposed to the way it was implemented from a technical perspective, in the sense that it was all done in a fully custom way instead of leveraging Microsoft.Extensions.Compliance.Redaction which to me would be the best course of action here: provide an extension point in the instrumentation that allows people to just attach redactors implemented using that abstraction.

Lastly, I disagree strongly with the recent change to v1.8.0 packages in nuget.org that were all tagged as "vulnerable". That to me is the same as saying Console.WriteLine is vulnerable because someone somewhere could provide a sensitive string to it. That's just crazy. That vulnerability warning forced me to bump the package version in a couple of our projects where we are using TreatWarningsAsErrors and I wish I didn't need to do that.

You guys are being too defensive on this.

[pellared]

It is an allowable change which is not seen as breaking from OpenTelemetry Specification point of view.
From: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/versioning-and-stability.md#semantic-conventions-stability

Things not listed in the above are not expected to remain stable via semantic convention and are allowed (or expected) to change. A few examples:
[...]

• The values of attributes

Closing this issue.