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

Release Hangfire instrumentation with OTel v1.6.0? #1500

Closed
gao-artur opened this issue Dec 18, 2023 · 4 comments · Fixed by #1502
Closed

Release Hangfire instrumentation with OTel v1.6.0? #1500

gao-artur opened this issue Dec 18, 2023 · 4 comments · Fixed by #1502
Labels
comp:instrumentation.hangfire Things related to OpenTelemetry.Instrumentation.Hangfire

Comments

@gao-artur
Copy link
Contributor

gao-artur commented Dec 18, 2023

I would like to release Hangfire instrumentation to get these changes: #1440, #1442.
Unfortunately, this repo was already updated to use OTel v1.7.0 #1486.
And OTel v1.7.0 was updated to use Microsoft.Extensions.* packages v8.0.0. open-telemetry/opentelemetry-dotnet#5020 + open-telemetry/opentelemetry-dotnet#5015

Unfortunately, we are not ready yet for .NET 8, and this dependency causes errors like this (we are using nuget CPM)

NU1109 Detected package downgrade: Microsoft.Extensions.Hosting.Abstractions from 8.0.0 to centrally defined 7.0.0. Update the centrally managed package version to a higher version.

Is it possible to downgrade the OpenTelemetry.Api.ProviderBuilderExtensions dependency in Hangfire instrumentation to 1.6.0 to create a compatible release?

And in general, this strategy of taking the latest versions of external packages sounds problematic to me. It leaves a lot of clients behind with old versions.

@gao-artur gao-artur added the comp:instrumentation.hangfire Things related to OpenTelemetry.Instrumentation.Hangfire label Dec 18, 2023
@Kielek
Copy link
Contributor

Kielek commented Dec 19, 2023

In this case, you do not need to update .NET SDK/runtime. It should be safe to just upgrade Microsoft.Extensions.Hosting.Abstractions package ans still target .NET7.

IMO it should be fine to release package with older version if upgrading package will be not an option . @fred2u, what do you think?

@gao-artur
Copy link
Contributor Author

Theoretically, you are right. Practically, it means starting the dependency hell. We are trying to keep SDK, runtime, and packages versions aligned to make sure everything is compatible and tested together. Also, it's not only Hosting.Abstractions. There are more, which means cascading updates of many dependencies, every one of which can bring new bugs. When updating OTel packages, I expect it to affect only OTel, but not configuration binding, for example. Lastly, when we need to add another MS package, we know exactly what version to use. Upgrading only part of the dependencies to v8.0 will make it less obvious, as any new dependency can cause additional cascade updates.

I'll open a corresponding issue in the main OTel repo to discuss this. In this issue, I only want to get the Hangfire release with a lower dependency version. I'll be happy to create a PR with a downgraded OTel version and another one to restore it.

@Kielek
Copy link
Contributor

Kielek commented Dec 19, 2023

@gao-artur, please do it. I suppose that @fred2u will agree on this.

Please remember about updating/reverting changelog.

@gao-artur
Copy link
Contributor Author

Relevant discussion in opentelemetry-dotnet repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.hangfire Things related to OpenTelemetry.Instrumentation.Hangfire
Projects
None yet
2 participants