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

.NET 8 build, CI, project targets, etc #4437

Closed
4 tasks done
alanwest opened this issue Apr 25, 2023 · 12 comments
Closed
4 tasks done

.NET 8 build, CI, project targets, etc #4437

alanwest opened this issue Apr 25, 2023 · 12 comments
Labels
enhancement New feature or request
Milestone

Comments

@alanwest
Copy link
Member

alanwest commented Apr 25, 2023

Checklist of infrastructure things to do in prep for .NET 8.

@alanwest alanwest added the enhancement New feature or request label Apr 25, 2023
@alanwest alanwest added this to the 1.6.0 milestone Apr 25, 2023
@alanwest alanwest modified the milestones: 1.6.0, 1.7.0 Jun 6, 2023
@vishweshbankwar
Copy link
Member

ASP.NET Core and HttpClient will generate metrics natively. Is there already a runtime issue tracking the work that will enable us to add the appropriate attributes to the metric

@alanwest We will not be able to leverage the metrics generated in these libraries in our instrumentation.

  1. It does not follow the OTel semantic conventions
  2. There is no extensibility to rename the dimensions to match the conventions.

I will open a separate issue to document this.

@eerhardt
Copy link
Contributor

Note that the ASP.NET Core metrics in .NET 8 have been renamed to align with OTel semantic conventions. See dotnet/aspnetcore#49743.

When using Open Telemetry on .NET 8, what is the recommended way of getting the http.server.* metrics? Which would I choose:

  • .AddAspNetCoreInstrumentation()
  • -or-
  • .AddMeter("Microsoft.AspNetCore.Hosting")

Should AddAspNetCoreInstrumentation() just call .AddMeter("Microsoft.AspNetCore.Hosting") when on net8.0, and when you are on a lower TFM, then it does what it does today (hook up a DiagnosticSource listener and forward the events to its own Meter)?

cc @JamesNK @utpilla @CodeBlanch

@vishweshbankwar
Copy link
Member

@eerhardt Thanks for the note. Something we need to evaluate if calling .AddMeter("Microsoft.AspNetCore.Hosting") is good enough on the OTel instrumentation library side. Planning to take a look once the changes are available in the preview/rc release (Current preview does not include these changes)

@eerhardt
Copy link
Contributor

Planning to take a look once the changes are available in the preview/rc release (Current preview does not include these changes)

If you want to try them out before 8.0-rc1 is officially shipped, you can get a nightly build at https://github.com/dotnet/installer#installers-and-binaries.

@cijothomas
Copy link
Member

#2994 can be resolved if we depend on built-in metrics from .NET8 onwards

@cijothomas
Copy link
Member

cijothomas commented Aug 23, 2023

#3948 (comment) can also be closed for .NET8+ as .NET built-in metrics also has a way to enrich the metrics it emits based on context. Need to check the details and see if they can be a viable alternative to whats asked in 3948

Edit: We cannot have diff. public API for each .NET version. So either the public API must be removed for all, or retained for all, and underlying implementation could somehow delegate to the built-in mechanism.

@cijothomas
Copy link
Member

https://github.com/dotnet/aspnetcore/blob/main/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs#L66-L75
@vishweshbankwar One thing to explore regd. duration.
Currently the duration is purely from Activity's duration. Looks like the new native metrics are outside of Activity start/stop, so if a user has expensive Enrich/Filter/Processors, the metric will measure that too. While it may be the true indication of actual latency experienced, it could surprise folks upgrading from old. Might be solvable with docs.

@cijothomas
Copy link
Member

@JamesNK @noahfalk @eerhardt
Could you comment on the plans for asp.net core/other metrics which currently adopted OTel Conventions, with regards to potential changes in OTel Conventions. Particularly in the case of breaking changes (it may be low probability once stable, but we never know!)?

Specifically - Would you adopt to the new changes in OTel conventions, even if they are breaking?

@vishweshbankwar
Copy link
Member

vishweshbankwar commented Aug 23, 2023

@JamesNK @noahfalk @eerhardt Could you comment on the plans for asp.net core/other metrics which currently adopted OTel Conventions, with regards to potential changes in OTel Conventions. Particularly in the case of breaking changes (it may be low probability once stable, but we never know!)?

Specifically - Would you adopt to the new changes in OTel conventions, even if they are breaking?

Have opened a separate issue to further discuss this. open-telemetry/opentelemetry-dotnet-contrib#1753

@sebader
Copy link

sebader commented Sep 20, 2023

Any update on .NET 8 support? The RC of .NET 8 is out since last week now.

@cijothomas
Copy link
Member

Any update on .NET 8 support? The RC of .NET 8 is out since last week now.

There is nothing preventing one to use .NET8 apps with current OTel. Are you facing issues with .NET 8 apps?

This issue is mostly to track build/ci changes, and there is no work specifically needed to support .NET 8 apps.

@Yun-Ting
Copy link
Contributor

Yun-Ting commented Sep 29, 2023

After #4876 got merged,
I'll use a follow-up PR to add net8.0 as target to w3c-trace-context-test.
there are issues in starting up docker image in net8: https://github.com/open-telemetry/opentelemetry-dotnet/actions/runs/6356983673/job/17267421505?pr=4876

Also should fix the below tests in ubuntu net8.0:
OpenTelemetry.Instrumentation.AspNetCore.Tests.BasicTests.DiagnosticSourceExceptionCallBackIsNotReceivedForExceptionsHandledInMiddleware
OpenTelemetry.Instrumentation.AspNetCore.Tests.BasicTests.RouteInformationIsNotAddedToRequestsOutsideOfMVC
OpenTelemetry.Instrumentation.AspNetCore.Tests.InProcServerTests.ExampleTest

build: https://github.com/open-telemetry/opentelemetry-dotnet/actions/runs/6358041216/job/17269881652?pr=4876

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants