Improve Class Loading metrics instrumentation

[lenin-jaganathan]

Describe the bug Currently, the JVM class unloading metric is being registered as a Function counter. The description says "The total number of classes unloaded since the Java virtual machine has started execution" which will not be true when this metric is used in a Step Meter Registry.

Environment

• Micrometer version - Any version
• Micrometer registry - Any Step Registry can be used
• OS: Any
• Java version: Any

To Reproduce How to reproduce the bug: Start the java application with the micrometer JVM binder enabled. Use any step registry to register these metrics. After one step is completed, the unloaded class becomes zero.

Expected behavior jvm.classes.unloaded - should represent the total number of classes unloaded since the Java virtual machine has started execution

[lenin-jaganathan]

~~I am not sure such behavior was created. But ideally, using the below snippet for class unloading will help solve this issue.~~

I see that jvm.classes.loaded metric is the number of classes that are loaded currently and this being a gauge makes sense.

jvm.classes.unloaded - is the total number of classes unloaded since the JVM has started, so this should not be a gauge but rather a counter.

Will it be informational to also expose "the total number of classes loaded" by the JVM?

[patpatpat123]

Upvoting this

[marcingrzejszczak]

Some questions:

• jvm.classes.loaded is a gauge
• jvm.classes.unloaded is a counter not a gauge, right?

So is this issue about adding the "the total number of classes loaded" ? Are you upvoting this @patpatpat123 ?

[patpatpat123]

Hello @marcingrzejszczak ,

Thank you for your input.

First of all, you are correct about the two metrics:

jvm_classes_loaded	        metric_type=gauge	        statistic=value
jvm_classes_unloaded	metric_type=counter	statistic=count

And yes, it would be great to have metrics unifying those, to allow building visuals such as a loaded vs. unloaded counter.

[github-actions[bot]]

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

[lenin-jaganathan]

I am voting for the below 2 things,

• Update the description of jvm.classes.unloaded to make the statement valid for Step registries. Currently, it says "classes loaded from JVM start" but this will not be true for a StepRegistry.
• Add a metric to report total classes loaded from ClassLoadingMXBean#getTotalLoadedClassCount. Ideally, this metric should be named "jvm.classes.loaded" but we already have a metric with the same name. We need to sort that out.

[marcingrzejszczak]

Thanks for the update. Are you willing to file a PR for this @lenin-jaganathan ?

[lenin-jaganathan]

I think there is an open question on how we handle the names,

• jvm.classes.unloaded - is a sorted thing. The opposite of this metric would be jvm.classes.loaded using info from ClassLoadingMXBean#getTotalLoadedClassCount but that name is already taken.

Today, we already have a metric named jvm.classes.loaded which reports the number of classes loaded currently. Ideally, this should have been jvm.classes.count or jvm.classes.current.count. I am not sure how will be able to handle this change. This will be a breaking change but in metric space emitting a metric with the same name but representing different info would be very dangerous.

Also see what OTEL does in this space.

[marcingrzejszczak]

We could do sth like this

• jvm.classes.unloaded for a counter of total unloaded classes
• jvm.classes.loaded would be the number of classes currently loaded
• jvm.classes.loaded.count could be the counter of total loaded classes

Now if someone wants to be OTel compliant they can create a MeterFilter and rename the metrics. If we decide one day that we want to migrate that will also mean that we would have to provide such a MeterFilter and give users a couple of minor versions to migrate to it and then change the names in core and deprecate the MeterFilter.

WDYT @shakuzen @jonatan-ivanov ?

[lenin-jaganathan]

I think if the names for classes loaded and unloaded don't match, it will cause a bit of confusion. However, since both of them are counters, I am not opposed to having a .count suffix for both of them.

[lenin-jaganathan]

I am proposing the below changes for the class loading metrics,

1. jvm.classes.loaded - Gauge - Gives the number of classes currently loaded (Keep it as is)
2. jvm.classes.unloaded.count - FunctionCounter - Tracks the number of classes unloaded (Rename existing metric jvm.classes.unloaded to jvm.classes.unloaded.count - this avoids confusion but will be a breaking change)
3. jvm.classes.loaded.count - FunctionCounter - Tracks the number of classes loaded (Generate a new metric)

@marcingrzejszczak / @shakuzen / @jonatan-ivanov Let me know what you guys think. Apart from the breaking change, there is no problem with the above. This is similar to what was proposed in but just that we add .count to the counter.

One another option would be to emit, jvm.classes.unloaded as an additional metric for one/two minor versions before removing it. But, that really does not mean a lot because unlike class deprecation I don't think people will be closely following the internal details.

I would call this metric name change out in release notes and move with option 1. Let me know what you guys think.

[shakuzen]

I've opened https://github.com/micrometer-metrics/micrometer/pull/5745 to address the description part of the issue, as I think that's a more straightforward change without open questions (though feel free to leave feedback there if there is any).

As for the new metric and naming, I'm not sure what is best. A breaking change is rather undesirable, but so is naming inconsistency. We'll discuss. I've relabeled the issue and updated the title since the description issue is being taken care of separately in the PR.

[shakuzen]

This has been resolved by https://github.com/micrometer-metrics/micrometer/pull/6682#discussion_r2315394094

[patpatpat123]

Many thanks for all of you.

To test this, I need to wait for this to be released right?

[shakuzen]

After it is merged, snapshots are published for each commit, but the M3 release was made shortly after the mentioned PR was merged, so the milestone is already available.