-
Notifications
You must be signed in to change notification settings - Fork 8
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
maint: Update OpenTelemetry instrumentation packages #334
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good, smoky boi not happy though
OTel .NET SDK v1.3.2 has been released, we should update to that as part of this PR too. |
Blegh, more smoke test failure, but this is ready and fully up to date |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running the aspnetcore example app locally and in Docker give an error:
Unhandled exception. System.TypeLoadException: Could not load type 'Microsoft.Extensions.DependencyInjection.TracerProviderBuilderServiceCollectionExtensions' from assembly 'OpenTelemetry, Version=1.0.0.0, Culture=neutral, PublicKeyToken=7bd6737fe5b67e3c'.
at Microsoft.Extensions.DependencyInjection.OpenTelemetryServicesExtensions.AddOpenTelemetryTracing(IServiceCollection services, Action`1 configure)
at Program.<Main>$(String[] args) in /app/examples/aspnetcore/Program.cs:line 11
at Program.<Main>(String[] args)
Good 'ole DI errors with mismatched assembly versions. Should be fixed now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cartermp beat me to it 🥇 . The one extra package in the example app was old.
Is this something we could or should add in our package? edit for clarity: "this" referring to the OpenTelemetry.Extensions.Hosting
package.
Can't due to NuGet packaging rules. It's considered pre-release, which would force us back to a pre-release if we took a dependency on it. |
Let's wait for 1.4.x to get released. There's some akward states in packages right now that make it not possible for us to upgrade without depending on that chain. |
Good news! Updated packages/samples and now we're a bunch of golden little goosies. Bad news! CI is sad about gpg (pronounced guh-puh-guh) signing in packaging. |
Try a rebase from main, I updated the snupkeg |
d5db8ce
to
6ef2963
Compare
fyi I am still working on the commoninsteumenttations package updates. Some msbuild fuckery is needed because of .NET 7 |
<ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp' And | ||
$([MSBuild]::VersionGreaterThanOrEquals($(_TargetFrameworkVersionWithoutV), '6.0'))"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, this is software gore, but it's the right way to do it.
Okay, this should be fully ready to review now that it's all updooted |
## Which problem is this PR solving? - release of new main package, including dependency updates and method changes from OTel v1.4.0 - release of new instrumentation package, including name change ## Short description of the changes - add changelog entries. - We renamed `AutoInstrumentations` to `CommonInstrumentations`; see #327 for more details. - OTel changed to `AddOpenTelemetry().WithTracing` and `AddOpenTelemetry().WithMetrics`; see #334 for more details. - Update versions of main package from 1.2.1 to 1.3.0 - Update versions of instrumentation packages from 0.26.1-beta to 0.27.0-beta - Update releasing notes with extra details for clarity Once the release is out, docs updates can be merged in for public docs and in-app onboarding (see #339 )
Which problem is this PR solving?
Bumps OpenTelemetry instrumentation packages to latest.
Short description of the changes