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

Add comments to package dependency to clarify the direction #4550

Merged
merged 7 commits into from
Jun 10, 2023

Conversation

reyang
Copy link
Member

@reyang reyang commented Jun 6, 2023

I think this will help to clarify the direction and also help to provide context/clarity to contributors (e.g. https://github.com/open-telemetry/opentelemetry-dotnet/pull/4548/files#r1220139771).

@reyang reyang requested a review from a team June 6, 2023 18:50
Directory.Packages.props Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 6, 2023

Codecov Report

Merging #4550 (6cfb898) into main (d57e6f8) will increase coverage by 0.00%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4550   +/-   ##
=======================================
  Coverage   85.11%   85.12%           
=======================================
  Files         320      320           
  Lines       12679    12679           
=======================================
+ Hits        10792    10793    +1     
+ Misses       1887     1886    -1     

see 6 files with indirect coverage changes

Copy link
Member

@alanwest alanwest left a comment

Choose a reason for hiding this comment

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

Approving as I'm not actually all that concerned if we always decide to stay on latest DiagnosticSource, but separately, I would like to better understand what is coming in .NET 8 that necessitates a DS version bump.

.NET 8 is improving dependency injection support for Meter/Instrument

It's my understanding that this support may actually be shipped in a new package Microsoft.Extensions.Diagnostics.

I guess the more interesting thing that is coming in DS v8 for our purposes is support for instrumentation scope attributes on Meter. Would be nice to see this for ActivitySource too.

@reyang reyang force-pushed the reyang/clarify-versioning-story branch from ea65253 to ad8bf04 Compare June 8, 2023 22:24
@reyang
Copy link
Member Author

reyang commented Jun 8, 2023

I guess the more interesting thing that is coming in DS v8 for our purposes is support for instrumentation scope attributes on Meter.

Thanks! I agree with what you mentioned here, updated the PR to provider better/accurate examples.

@reyang
Copy link
Member Author

reyang commented Jun 8, 2023

I would like to better understand what is coming in .NET 8 that necessitates a DS version bump.

Here is a subset (scoped to metrics) https://github.com/dotnet/runtime/issues?q=is%3Aopen+label%3Aarea-System.Diagnostics.Metric+milestone%3A8.0.0 FYI @alanwest

@utpilla utpilla merged commit 3274c43 into open-telemetry:main Jun 10, 2023
mfogliatto pushed a commit to mfogliatto/opentelemetry-dotnet that referenced this pull request Jun 10, 2023
@reyang reyang deleted the reyang/clarify-versioning-story branch October 5, 2023 03:53
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.

6 participants