[Instrumentation.Runtime] Reintroduce current heap size metric #683

lachmatt · 2022-10-10T10:19:57Z

Fixes #682.

Changes

Add process.runtime.dotnet.gc.objects.size instrument that reports count of bytes currently in use by objects in the GC heap that haven't been collected yet (excludes fragmentation and other GC committed memory pools), and uses API available on all targets.

codecov · 2022-10-10T10:28:26Z

Codecov Report

Merging #683 (0e460da) into main (b3a921a) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #683      +/-   ##
==========================================
+ Coverage   77.56%   77.58%   +0.02%     
==========================================
  Files         176      176              
  Lines        5308     5313       +5     
==========================================
+ Hits         4117     4122       +5     
  Misses       1191     1191

Impacted Files	Coverage Δ
...elemetry.Instrumentation.Runtime/RuntimeMetrics.cs	`85.61% <100.00%> (+0.51%)`	⬆️

reyang · 2022-10-11T17:44:00Z

src/OpenTelemetry.Instrumentation.Runtime/RuntimeMetrics.cs

+            "process.runtime.dotnet.gc.heap.live",
+            () => GC.GetTotalMemory(false),
+            unit: "bytes",
+            description: "Count of bytes currently in use by live objects in the GC heap.");


I guess this is the estimated bytes allocated, not necessarily "used by live objects" (e.g. I think it will contain fragmentation/gaps that are not connected to any live objects").

Maybe we can borrow some wording from https://learn.microsoft.com/en-us/dotnet/api/system.gc.gettotalmemory?view=net-6.0#returns, e.g. "A number that is the best available approximation of the number of bytes currently allocated in managed memory."

+1 to improve wording to indicate this is an approximation.

Current wording comes from here.

I guess this is the estimated bytes allocated, not necessarily "used by live objects" (e.g. I think it will contain fragmentation/gaps that are not connected to any live objects").

Unless I'm looking at the wrong place, looking at the implementation (e.g gc.cpp, line 44841 for 6.0.0), fragmentation is specifically excluded.

I preferred current wording, because it is more specific.
If wording from docs is preferred, I will use that.

Thanks @lachmatt! Let me consult the owner of GC and see if we can improve the description here.

Sorry for the delay, I've been jumping between a number of different issues. I've been chatting more with Maoni and we've got some misgivings about using for using GC.GetTotalMemory() in runtime's own counter. My plan for the moment is to write up a proposal and open a runtime GitHub issue to discuss deprecating the gc-heap-size counter. As part of that we would change the tutorial to reference the gc-committed counter instead. If the guidance in that tutorial changed would there be any other motivation to add this counter or would it now be moot?

This is the issue I just opened in the runtime repo discussing moving/removing/hiding the gc-heap-size counter: dotnet/runtime#77530

As @lachmatt mentioned the only downside to gc-committed is that it requires .NET 6.0 so no .NET Framework support and we still see many users on .NET Framework. I'm not concerned about .NET Core 3.1 given its upcoming EOL.

In the same line with proposals 1 & 5 on dotnet/runtime#77530 we could document that the counter is available for legacy purposes and that gc-committed is preferred to track actual memory used by GC.

Thinking out loud: how much gc-committed diverges from the native perf counter tracking MEM_COMMIT for the typical .NET Framework application?

If the overall consensus is that adding gc-heap-size is not desirable. The docs could guide users that lack the gc-committed to the proper native counter, and explain what is expected to be different with it.

Depending on the app, total committed memory and GC committed memory could be very different numbers. I've been continuing to noodle on this and the feedback from dotnet/runtime#77530. I think the main commonality from proposals (1) and (5) is that neither of them gets rid of the heap size metric, they just try to re-position it to reduce accidental misuse. If this is will be the only metric we have for .NET Framework then maybe rather than avoiding it we just be careful with naming it. From my perspective, the key distinction of GetTotalMemory() isn't that it updates between GCs, it is that it doesn't include fragmentation and other GC committed memory pools. Perhaps if we named it process.runtime.dotnet.gc.heap.total_objects_size that would make it clearer what this metric is really measuring.

reyang · 2022-11-08T16:27:56Z

src/OpenTelemetry.Instrumentation.Runtime/RuntimeMetrics.cs

@@ -48,6 +48,12 @@ static RuntimeMetrics()
            () => GetGarbageCollectionCounts(),
            description: "Number of garbage collections that have occurred since process start.");

+        MeterInstance.CreateObservableUpDownCounter(
+            "process.runtime.dotnet.gc.heap.total_objects_size",


This is very inconsistent with other metrics names (e.g.

opentelemetry-dotnet-contrib/src/OpenTelemetry.Instrumentation.Runtime/RuntimeMetrics.cs

Line 53 in 005fdb7

"process.runtime.dotnet.gc.allocations.size",

) in the existing library. Consider:

Suggested change

"process.runtime.dotnet.gc.heap.total_objects_size",

"process.runtime.dotnet.gc.live_objects.size",

renamed in 25e9cf8

'live' usually means that an object is reachable via references from a root, but that isn't necessarily true for these objects at the current instant in time. We know these objects haven't been collected yet, but they may already be unreachable. We can brainstorm different names but I would avoid "live". For example "process.runtime.dotnet.gc.objects.size"?

'live' usually means that an object is reachable via references from a root, but that isn't necessarily true for these objects at the current instant in time. We know these objects haven't been collected yet, but they may already be unreachable. We can brainstorm different names but I would avoid "live". For example "process.runtime.dotnet.gc.objects.size"?

Sounds good to me, and the name "process.runtime.dotnet.gc.objects.size" is aligned with the existing ones.

@noahfalk would you help to keep an eye on the instrument description? Current it says "live objects" which needs to be adjusted based on your input.

renamed in 48e021b

src/OpenTelemetry.Instrumentation.Runtime/RuntimeMetrics.cs

reyang

LGTM.

cijothomas · 2022-11-15T17:31:21Z

src/OpenTelemetry.Instrumentation.Runtime/CHANGELOG.md

@@ -34,6 +34,9 @@

  This does not affect applications targeting .NET Framework.

+* Add "process.runtime.dotnet.gc.objects.size" metric


https://github.com/open-telemetry/opentelemetry-dotnet-contrib/tree/main/src/OpenTelemetry.Instrumentation.Runtime#processruntimedotnetgcheapsize
Could you update the readme file as well?

would be also add a note about how is it different from the gc.heap.size (the fact that this exclude fragmentation, and works anytime even before the 1st GC event itself etc.) A lot such useful info was in this PR as comments. Its good to get the gist into the readme file itself to not lose them :)

cijothomas

Thanks.
The changes look good. Requested to update readme file as well. (It could be done as a follow PR as well)

cijothomas · 2022-11-15T17:34:30Z

@lachmatt could you also update PR desc to reflect the final content of the PR?

lachmatt · 2022-11-16T12:11:20Z

@lachmatt could you also update PR desc to reflect the final content of the PR?

Updated.

Thanks. The changes look good. Requested to update readme file as well. (It could be done as a follow PR as well)

I will do that in a separate PR.

cijothomas · 2022-11-16T15:39:35Z

Merging. A follow up is needed to update readme.
Also, if there are any feedbacks about the instrument description, please reply here/open a new issue.

noahfalk

LGTM 👍

add heap size metric

968f3b9

lachmatt requested a review from a team October 10, 2022 10:19

update changelog.md

a166bd0

Kielek approved these changes Oct 10, 2022

View reviewed changes

lachmatt changed the title ~~[Instrumentation.Runtime] Reintroduce heap size metric~~ [Instrumentation.Runtime] Reintroduce current heap size metric Oct 10, 2022

pellared approved these changes Oct 10, 2022

View reviewed changes

add arg names for consistency

ae7f221

utpilla added the comp:instrumentation.runtime Things related to OpenTelemetry.Instrumentation.Runtime label Oct 11, 2022

Merge branch 'main' into gc-heap-size-metric

b814127

reyang reviewed Oct 11, 2022

View reviewed changes

Merge branch 'main' into gc-heap-size-metric

3077b2c

reyang mentioned this pull request Oct 20, 2022

Add more .NET CLR related metrics to make it similar to .NET Framework #713

Open

lachmatt added 2 commits November 8, 2022 08:28

Merge branch 'main' into gc-heap-size-metric

b1daf99

Merge branch 'main' into gc-heap-size-metric

ab3e327

Kielek approved these changes Nov 8, 2022

View reviewed changes

reyang reviewed Nov 8, 2022

View reviewed changes

apply suggested rename

25e9cf8

reyang reviewed Nov 9, 2022

View reviewed changes

src/OpenTelemetry.Instrumentation.Runtime/RuntimeMetrics.cs Outdated Show resolved Hide resolved

lachmatt added 2 commits November 15, 2022 09:53

additional renames

48e021b

Merge branch 'main' into gc-heap-size-metric

1f514e6

reyang approved these changes Nov 15, 2022

View reviewed changes

Merge branch 'main' into gc-heap-size-metric

0e460da

cijothomas reviewed Nov 15, 2022

View reviewed changes

cijothomas approved these changes Nov 15, 2022

View reviewed changes

cijothomas merged commit 679d871 into open-telemetry:main Nov 16, 2022

lachmatt deleted the gc-heap-size-metric branch November 16, 2022 15:40

noahfalk reviewed Nov 16, 2022

View reviewed changes

lachmatt mentioned this pull request Nov 17, 2022

[Instrumentation.Runtime] Update readme after inclusion of process.runtime.dotnet.gc.objects.size #772

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Instrumentation.Runtime] Reintroduce current heap size metric #683

[Instrumentation.Runtime] Reintroduce current heap size metric #683

lachmatt commented Oct 10, 2022 •

edited

Loading

codecov bot commented Oct 10, 2022 •

edited

Loading

reyang Oct 11, 2022

reyang Oct 11, 2022

cijothomas Oct 11, 2022

lachmatt Oct 12, 2022 •

edited

Loading

reyang Oct 12, 2022

noahfalk Oct 27, 2022

noahfalk Oct 27, 2022

pjanotti Nov 1, 2022

pjanotti Nov 1, 2022

noahfalk Nov 1, 2022

reyang Nov 8, 2022

lachmatt Nov 9, 2022

noahfalk Nov 9, 2022

reyang Nov 9, 2022 •

edited

Loading

lachmatt Nov 15, 2022

reyang left a comment

cijothomas Nov 15, 2022

cijothomas Nov 15, 2022

cijothomas left a comment

cijothomas commented Nov 15, 2022

lachmatt commented Nov 16, 2022 •

edited

Loading

cijothomas commented Nov 16, 2022

noahfalk left a comment

	"process.runtime.dotnet.gc.heap.total_objects_size",
	"process.runtime.dotnet.gc.live_objects.size",

		@@ -34,6 +34,9 @@

		This does not affect applications targeting .NET Framework.

		* Add "process.runtime.dotnet.gc.objects.size" metric

[Instrumentation.Runtime] Reintroduce current heap size metric #683

[Instrumentation.Runtime] Reintroduce current heap size metric #683

Conversation

lachmatt commented Oct 10, 2022 • edited Loading

Changes

codecov bot commented Oct 10, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lachmatt Oct 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyang Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cijothomas left a comment

Choose a reason for hiding this comment

cijothomas commented Nov 15, 2022

lachmatt commented Nov 16, 2022 • edited Loading

cijothomas commented Nov 16, 2022

noahfalk left a comment

Choose a reason for hiding this comment

lachmatt commented Oct 10, 2022 •

edited

Loading

codecov bot commented Oct 10, 2022 •

edited

Loading

lachmatt Oct 12, 2022 •

edited

Loading

reyang Nov 9, 2022 •

edited

Loading

lachmatt commented Nov 16, 2022 •

edited

Loading