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

Consider changes to process.runtime.jvm.classes.* #3429

Closed
trask opened this issue Apr 21, 2023 · 16 comments
Closed

Consider changes to process.runtime.jvm.classes.* #3429

trask opened this issue Apr 21, 2023 · 16 comments
Assignees
Labels
area:semantic-conventions Related to semantic conventions spec:metrics Related to the specification/metrics directory

Comments

@trask
Copy link
Member

trask commented Apr 21, 2023

Currently these metrics are defined under process.runtime.jvm.classes.*:

  • process.runtime.jvm.classes.loaded
  • process.runtime.jvm.classes.unloaded
  • process.runtime.jvm.classes.current_loaded

One question which was raised is whether we need all three since current = loaded - unloaded, but we found reference to prior discussion and I believe the answer to that is summarized by @jack-berg's #2549 (comment)

we can't atomically collect loaded and unloaded so most accurate to report this separately

Another question is whether there could be clearer naming to reflect that loaded and unloaded are cumulative totals over time.

@trask trask added area:semantic-conventions Related to semantic conventions spec:metrics Related to the specification/metrics directory labels Apr 21, 2023
@trask trask assigned trask and unassigned yurishkuro Apr 21, 2023
@breedx-splk
Copy link
Contributor

My $0.02:

One question which was raised is whether we need all three [...]

I think it does make sense to have all 3 in the spec. I'd bet there are two main uses cases for these metrics -- troubleshooting time spent doing large number of class loads over a long time (esp dynamic jvm languages) , and troubleshooting large number of loaded/resident classes (like when comparing with a heap dump). Just because the the resident count can be calculated, it doesn't mean it will always be convenient or easy for a user. For the implementer, I think this also suggests that loaded/unloaded are observed in lockstep...which I also doubt is very easy to guarantee.

I think having all 3 gives implementers a bit more flexibility. I suppose a downside is that it opens up the possibility for the math to not work out, which could be confusing for some users (and require an explanation from implementers).

Another question is whether there could be clearer naming to reflect that loaded and unloaded are cumulative totals over time.

Maybe the type being UpCounter is enough to make that clear. The difference between loaded and current_loaded might be clarified by changing process.runtime.jvm.classes.current_loaded to process.runtime.jvm.classes.resident? In my brain, "resident" more strongly suggests "presently" and doesn't carry the "loaded" similarity.

@roberttoyonaga
Copy link
Contributor

Another question is whether there could be clearer naming

Someone mentioned this format in the WG, which I think is quite clear:
process.runtime.jvm.classes.total_loaded
process.runtime.jvm.classes.total_unloaded
process.runtime.jvm.classes.current_loaded

@vijaysun-omr
Copy link

vijaysun-omr commented Apr 25, 2023

I agree with "For the implementer, I think this also suggests that loaded/unloaded are observed in lockstep...which I also doubt is very easy to guarantee" by @breedx-splk If these two values are read at different points of time, they may indeed not be in lock step and it would be an unnecessary burden on the JVM to require it to do better. I agree that we should just document the possibility in this case.

@jack-berg
Copy link
Member

Someone mentioned this format in the WG, which I think is quite clear:
process.runtime.jvm.classes.total_loaded
process.runtime.jvm.classes.total_unloaded
process.runtime.jvm.classes.current_loaded

This intersects with #3457. Since total_loaded and total_unloaded are monatonic sums, they would receive the _total suffix when translated to prometheus, resulting in total_loaded_total and total_unloaded_total. Should probably avoid the "total" keyword in counter metric names.

@trask
Copy link
Member Author

trask commented Apr 28, 2023

looking at system.processes.count and system.processes.created, we could:

  • process.runtime.jvm.classes.loaded
  • process.runtime.jvm.classes.unloaded
  • process.runtime.jvm.classes.count

(there's an open question about .count vs .total in #3457)

@trask
Copy link
Member Author

trask commented May 4, 2023

Here's latest proposal from today's SIG meeting:

  • process.runtime.jvm.classes.resident (or .active)
  • process.runtime.jvm.classes.loaded
  • process.runtime.jvm.classes.unloaded

@trask
Copy link
Member Author

trask commented May 5, 2023

How about also process.runtime.jvm.threads.count --> process.runtime.jvm.threads.active?

@trask
Copy link
Member Author

trask commented May 5, 2023

process.runtime.jvm.buffer.count -> process.runtime.jvm.buffer.active?

@trask
Copy link
Member Author

trask commented May 5, 2023

what about mix of pluralization between .classes, .threads, .buffer?

compare to system.processes.* (any others?)

spec guidance here: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/README.md#pluralization suggests to use .buffers

@trask
Copy link
Member Author

trask commented May 8, 2023

I split out the .buffer discussion into #3483

@trask
Copy link
Member Author

trask commented May 8, 2023

How about also process.runtime.jvm.threads.count --> process.runtime.jvm.threads.active?

I realized a problem with this is that it sounds like the number of threads that are "actively running" (i.e. RUNNABLE)

@trask
Copy link
Member Author

trask commented May 8, 2023

I think I've convinced myself that process.runtime.jvm.classes.count is not confusing 😅

process.runtime.jvm.classes.count is the "count of classes in the JVM"

this works for other cases too, e.g.

  • process.runtime.jvm.threads.count is the "count of threads in the JVM"
  • system.processes.count is the "count of processes in the system", and

also, in prometheus, _total will get appended to the monotonic sums, so we end up with

  • process.runtime.jvm.classes.count
  • process.runtime.jvm.classes.loaded_total
  • process.runtime.jvm.classes.unloaded_total

@trask
Copy link
Member Author

trask commented May 11, 2023

process.runtime.jvm.classes.residual?

@trask
Copy link
Member Author

trask commented May 11, 2023

For process.runtime.jvm.classes.current_loaded:

If metric namespaces can be metric names:

  • process.runtime.jvm.classes

If not:

  • process.runtime.jvm.classes.count

@trask trask moved this to Blocked on general spec issue in Spec: JVM runtime metric stability May 23, 2023
@trask
Copy link
Member Author

trask commented May 26, 2023

We also have

@trask
Copy link
Member Author

trask commented May 26, 2023

Closing, results of this discussion are now captured in open-telemetry/semantic-conventions#59 and open-telemetry/semantic-conventions#60

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions spec:metrics Related to the specification/metrics directory
Projects
Development

No branches or pull requests

6 participants