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

Bump OpenTelemetry .NET Automatic Instrumentation to 1.3.0 #2538

Merged
merged 4 commits into from
Jan 23, 2024

Conversation

Kielek
Copy link
Contributor

@Kielek Kielek commented Jan 18, 2024

Description:
Bump OpenTelemetry .NET Automatic Instrumentation to 1.3.0.
As discussed in #2382. Entry for changelog is not needed.

Link to tracking Issue:
https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/releases/tag/v1.3.0

Testing: CI

Documentation:
See changelog in https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/releases/tag/v1.3.0

@Kielek Kielek requested a review from a team as a code owner January 18, 2024 11:01
Copy link
Contributor

@lachmatt lachmatt left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

Blocking again because I want to discuss the impact of this change (like we did for Java).

@Kielek reading the changelog it looks like the agent now installs the 1.7 instrumentation libraries by default, which include all the stable HTTP semantic convention breaking changes, am I understanding this correctly?

I am worried about how this change in the operator will impact users. Users using the operator to inject the dotnet instrumentation have 2 layers of obfuscation (the operator and the dotnet auto instrumentation) to understand before they realize that this change breaks all their attributes. I am also worried about understanding the impact because there wasn't a major version change.

I see that dotnet is not supporting the old version going forward like Java is, but I'd like us (the Operator) to think about how we can help users understand how impactful these stable HTTP semconv changes are to their telemetry.

@jaronoff97
Copy link
Contributor

Open issue about the related breaking changes for this release open-telemetry/semantic-conventions#534

@pavolloffay
Copy link
Member

@TylerHelmuth can this be merged? The default will not be changed #2543

@Kielek
Copy link
Contributor Author

Kielek commented Jan 19, 2024

@TylerHelmuth, http/network semantic convention was changed for ASP.NET, ASP.NET Core and HTTP Instrumentation. There is no support for old sem. conv.

Theoretically (I know that the real world is different), it is not a problem:

  1. Previously http and network sem. conv. was not stable. There were no breaking changes after stabilization.
  2. There were no stable instrumentation libraries in OTel for .NET. The version with new sem. conv. is first stable release.
  3. All instrumentations are/were marked as not stable in automatic instrumentation. As long as they are not stable, we can release minor versions.
  4. Operator is not stable (<1.0.0).

What is more, OTel Sem. conv. will accept a lot of breaking changes in the future for non-stable parts. If e.g. SQL/DB sem/ conv. will be marked as stable, do you recommend to release Java 3.0? If not, why? It is serious question, I do not have a good answer. Do you have a plan to block such upgrades here?

What's about dropping support for .NET6 and .NET7. Both of them will reach EOL this year. .NET team decided to support only currently supported versions (without rising major version). Do you have plan to also block the upgrade versions here?

IMO all these cases touches same issue problem and I do not have good answer. Still, theoretically we do not do anything against policies and we can make upgrades with clear statement in changelog.

@TylerHelmuth
Copy link
Member

TylerHelmuth commented Jan 19, 2024

@Kielek I blocked the PR only to force us to have a discussion. I agree that there will be many breaking changes from the semantic convention in the future, so now is the time for us (all of OpenTelemetry) to figure out how we will help our users navigate these impactful changes. Breaking changes to the semconv are insidious: unlike a breaking change in code, I could take libraries with the new semconv, my code would compile, my unit tests would pass, and I could start emitting new attributes without realizing it - potentially causing alerts to not fire when they should.

The reality is that the HTTP semantic conventions are incredibly widely used, despite the fact that they were not marked stable. For this reason we need to do everything we can to help users understand the impact when they take upgrades. I blocked the PR so that the Operator team could have a discussion because I believe we need to figure out a solution to make it extremely apparent to users the impact of taking the new stable semantic conventions (#2542). Until the operator team has a solution for our stuff I don't feel comfortable bumping the default versions.

I also believe that the OpenTelemetry Community owes its end users extreme clarity on these breaking changes, even if that costs us a maintenance burden in the meantime (open-telemetry/semantic-conventions#534). As an end users, I should be able to look at any changelog/release and instantly comprehend that there are breaking changes that affect me. I should also be able to look at any instrumentation documentation and instantly understand if it is emitting stable http semantic conventions or allows the use of the OTEL_SEMCONV_STABILITY_OPT_IN env variable. I believe we are not meeting that expectation at this time.

With all due respect to the amazing work the .NET team does, I'd like to criticize these release notes specifically. In the 1.3 release notes there are no mentions of the impact of that comes along with the 1.7 instrumentation libraries and the 1.7 release notes linked from the 1.3 release notes do not mention any breaking changes either. I understand it is a users responsibility to understand the libraries they are using, but I think we can do more to make it clearer the impacts that the libraries bring with them.

We'll be breaking lots and lots more semantic conventions are OTel matures (hopefully they won't all be as impactful as HTTP but they probably will be), so I'd like to see the community implement some process/system/template/expectation that helps us evangelize the changes in a consistent and clear way.

@Kielek
Copy link
Contributor Author

Kielek commented Jan 22, 2024

@TylerHelmuth, thanks for the explanation of your perspective.

I fully agree, that we have an issue with the sem-conv. even if all changes was made with full respect to the definition/release process (it includes .NET and .NET Auto).

If we speaking about transitively copying changelog/documentation from dependencies to the derived components, I do not like the idea. In Auto-Instrumentation we keep links updated in our documentation. See example for: https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/blob/v1.3.0/docs/config.md#metrics-instrumentations.

If you think, that it is worth to add changelog for this change in operator, I will be happy to do this. I would keep it rather minimal that full copy of the external changelogs.

Eg:

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: enhancement

# The name of the component, or a single word describing the area of concern, (e.g. operator, target allocator, github action)
component: autoinstrumentation

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Bump OpenTelemetry .NET Automatic Instrumentation to 1.3.0

# One or more tracking issues related to the change
issues: [2538]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext: This version brings changes stable semantic convention for network and HTTP. It includes new metrics and attributes names.

Please let me know if you want to include it.

@pavolloffay
Copy link
Member

pavolloffay commented Jan 22, 2024

@Kielek please add the changelog with the following changes

change_type: breaking
note: Publish OpenTelemetry .NET Automatic Instrumentation to 1.3.0

@Kielek Kielek changed the title [chore] Bump OpenTelemetry .NET Automatic Instrumentation to 1.3.0 Bump OpenTelemetry .NET Automatic Instrumentation to 1.3.0 Jan 23, 2024
component: autoinstrumentation

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Bump OpenTelemetry .NET Automatic Instrumentation to 1.3.0
Copy link
Member

Choose a reason for hiding this comment

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

this should be Publish I will fix it in the follow up PR

@pavolloffay pavolloffay merged commit c76462e into open-telemetry:main Jan 23, 2024
28 checks passed
@Kielek Kielek deleted the patch-1 branch January 23, 2024 09:10
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
…elemetry#2538)

* Bump OpenTelemetry .NET Automatic Instrumentation to 1.3.0

* add changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants