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

[Instrumentation.Runtime] Update OTel API version to 1.4.0-beta.2 and change runtime metrics type to ObservableUpDownCounter #675

Merged
merged 15 commits into from
Oct 21, 2022

Conversation

xiang17
Copy link
Contributor

@xiang17 xiang17 commented Oct 4, 2022

Update OTel API version to 1.4.0-beta.2 and change runtime metrics type to ObservableUpDownCounter now that it's ready. (Released in 1.4.0-beta.1: https://github.com/open-telemetry/opentelemetry-dotnet/releases/tag/core-1.4.0-beta.1)

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #

@xiang17 xiang17 requested a review from a team October 4, 2022 23:07
@codecov
Copy link

codecov bot commented Oct 5, 2022

Codecov Report

Merging #675 (967011e) into main (52585a1) will not change coverage.
The diff coverage is 85.71%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #675   +/-   ##
=======================================
  Coverage   77.74%   77.74%           
=======================================
  Files         176      176           
  Lines        5343     5343           
=======================================
  Hits         4154     4154           
  Misses       1189     1189           
Impacted Files Coverage Δ
...elemetry.Instrumentation.Runtime/RuntimeMetrics.cs 85.10% <85.71%> (ø)

Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

I have doubts if the beta version should be included into the contrib.
The changes does not looks huge and probably could be included together with 1.4.0 release.

src/OpenTelemetry.Instrumentation.Runtime/CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Piotr Kiełkowicz <pkiekowicz@splunk.com>
@xiang17
Copy link
Contributor Author

xiang17 commented Oct 5, 2022

I have doubts if the beta version should be included into the contrib. The changes does not looks huge and probably could be included together with 1.4.0 release.

I have similar doubts on how to integrate beta version. If there happen to be any changes needed for the stable version, we'd need to create a branch from a past commit and maintain two branches. However, there are some benefits for doing it early:

  1. More changes coming to this PR to upgrade to 1.4.0, because netcoreapp3.1 should be removed. I need to remove netcoreapp3.1 in TargetFrameworks, and potentially in GitHub actions. Refer to warnings in the build:
/home/runner/.nuget/packages/system.diagnostics.diagnosticsource/7.0.0-rc.1.22426.10/buildTransitive/netcoreapp2.0/System.Diagnostics.DiagnosticSource.targets(4,5): warning : System.Diagnostics.DiagnosticSource 7.0.0-rc.1.22426.10 doesn't support netcoreapp3.1 and has not been tested with it. Consider upgrading your TargetFramework to net6.0 or later. You may also set <SuppressTfmSupportBuildWarnings>true</SuppressTfmSupportBuildWarnings> in the project file to ignore this warning and attempt to run in this unsupported configuration at your own risk. [/home/runner/work/opentelemetry-dotnet-contrib/opentelemetry-dotnet-contrib/src/OpenTelemetry.Instrumentation.Runtime/OpenTelemetry.Instrumentation.Runtime.csproj]
/home/runner/.nuget/packages/system.diagnostics.diagnosticsource/7.0.0-rc.1.22426.10/buildTransitive/netcoreapp2.0/System.Diagnostics.DiagnosticSource.targets(4,5): warning : System.Diagnostics.DiagnosticSource 7.0.0-rc.1.22426.10 doesn't support netcoreapp3.1 and has not been tested with it. Consider upgrading your TargetFramework to net6.0 or later. You may also set <SuppressTfmSupportBuildWarnings>true</SuppressTfmSupportBuildWarnings> in the project file to ignore this warning and attempt to run in this unsupported configuration at your own risk. [/home/runner/work/opentelemetry-dotnet-contrib/opentelemetry-dotnet-contrib/test/OpenTelemetry.Instrumentation.Runtime.Tests/OpenTelemetry.Instrumentation.Runtime.Tests.csproj]
  1. Early access to anyone who needs to test/preview/integrate with the UpDownCounter or OTel 1.4.0 version.

@utpilla Do you have any suggestion? Especially for removing netcoreapp3.1 in Github action, it may apply to all projects in this repo.

@cijothomas
Copy link
Member

I have doubts if the beta version should be included into the contrib. The changes does not looks huge and probably could be included together with 1.4.0 release.

Each component can chose the version they want. We have no reason to prevent any component from moving to a newer version.

@@ -90,8 +89,7 @@ static RuntimeMetrics()
// Either Environment.Version is not 6 or (it's 6 but internal API GC.GetGenerationSize is valid)
if (!isCodeRunningOnBuggyRuntimeVersion || getGenerationSize != null)
{
// TODO: change to ObservableUpDownCounter
MeterInstance.CreateObservableGauge(
MeterInstance.CreateObservableUpDownCounter(
Copy link
Member

Choose a reason for hiding this comment

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

Do you think this is a breaking change? For sure OTLP Exporters see dif. data type now, but exporter like Prometheus would be fine...
@alanwest Can you check from NR stand point, if this will be breaking for anyone sending to NR via OTLP?

Copy link
Member

Choose a reason for hiding this comment

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

Apologies for delay. No this is not breaking for New Relic. NR has its own gauge data type. Both OTLP gauges and OTLP non-monotonic sums are transformed to a NR gauge, so there will be no difference for end users querying this data in NR.

Though, it may be possible this could be breaking for other backends.

Copy link
Member

Choose a reason for hiding this comment

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

We can bring this to the next SIG to make a call about do we need to major version bump or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lachmatt checked changes with Splunk OTLP endpoint and it is working correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW I would consider it as a bugfix not a breaking change. There were no possibility to set it correctly with older dependencies.

@utpilla utpilla added the comp:instrumentation.runtime Things related to OpenTelemetry.Instrumentation.Runtime label Oct 10, 2022
@utpilla utpilla changed the title Update OTel API version to 1.4.0-beta.1 and change runtime metrics type to ObservableUpDownCounter [Instrumentation.Runtime] Update OTel API version to 1.4.0-beta.1 and change runtime metrics type to ObservableUpDownCounter Oct 10, 2022
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added Stale and removed Stale labels Oct 18, 2022
@xiang17 xiang17 changed the title [Instrumentation.Runtime] Update OTel API version to 1.4.0-beta.1 and change runtime metrics type to ObservableUpDownCounter [Instrumentation.Runtime] Update OTel API version to 1.4.0-beta.2 and change runtime metrics type to ObservableUpDownCounter Oct 20, 2022
@@ -301,6 +297,23 @@ metric is available in the .NET version you are running.
Some GC related metrics are unavailable until at least one garbage collection
has occurred.

## Note
Copy link
Member

Choose a reason for hiding this comment

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

do we need it here?

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

changes look good!
Based on most comments I am proposing to release this as minor version bump only. But will discuss this in next SIG meeting to finalize. (Stable cannot anyway occur before 1.4.0 of the API itself is stable, expected November 2022)

@cijothomas cijothomas merged commit bb9462c into open-telemetry:main Oct 21, 2022
@xiang17 xiang17 deleted the xiang17/UpDownCounter branch November 22, 2022 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.runtime Things related to OpenTelemetry.Instrumentation.Runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants