add file descriptor metrics #11876
add file descriptor metrics #11876
Conversation
| if (openFileDescriptorCount != null) { | ||
| observables.add( | ||
| meter | ||
| .gaugeBuilder("os.file.descriptor.open") |
There was a problem hiding this comment.
https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/receiver/hostmetricsreceiver/internal/scraper/processscraper/documentation.md#processopen_file_descriptors
defines open file descriptor count as an up down counter. os.file.descriptor.open does not align with existing metric names, perhaps jvm.process.open_file_descriptors.
| observables.add( | ||
| meter | ||
| .gaugeBuilder("os.file.descriptor.open") | ||
| .setDescription("number of open file descriptors") |
There was a problem hiding this comment.
all other descriptions start with a capital letter and end with dot. Host metric receiver uses the following description Number of file descriptors in use by the process.
| meter | ||
| .gaugeBuilder("os.file.descriptor.open") | ||
| .setDescription("number of open file descriptors") | ||
| .setUnit("{file}") |
There was a problem hiding this comment.
I'd use file_descriptor though hostmerics receiver uses {count} @trask any suggestions?
There was a problem hiding this comment.
There was a problem hiding this comment.
@xiangtianyu is this the same as https://github.com/open-telemetry/semantic-conventions/blob/main/docs/system/process-metrics.md#metric-processopen_file_descriptorcount?
This looks the same
| * This class is internal and is hence not for public use. Its APIs are unstable and can change at | ||
| * any time. | ||
| */ | ||
| public class FileDescriptorMethods { |
There was a problem hiding this comment.
this is mostly copy paste from https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/runtime-telemetry/runtime-telemetry-java8/library/src/main/java/io/opentelemetry/instrumentation/runtimemetrics/java8/internal/CpuMethods.java You could add your methods in that class and rename the class appropriately.
| if (maxFileDescriptorCount != null) { | ||
| observables.add( | ||
| meter | ||
| .gaugeBuilder("os.file.descriptor.max") |
There was a problem hiding this comment.
Host metrics receiver does not have this. It is similar to jvm.memory.limit and jvm.buffer.memory.limit that we already have. I'd use limit instead of max in the metric name. The descriptions of the existing limit metrics are Measure of max obtainable memory. and Measure of total memory capacity of buffers. The description of this metric should follow similar style.
There was a problem hiding this comment.
+1 on .limit
@xiangtianyu are these process metrics, or os-wide metrics? can you open an issue in https://github.com/open-telemetry/semantic-conventions to propose any new metrics, and add a comment in the code pointing to that issue?
There was a problem hiding this comment.
Looking at https://github.com/openjdk/jdk/blob/2f2223d7524c4405cc7ca6ab77da62016bbfa911/src/jdk.management/unix/native/libmanagement_ext/OperatingSystemImpl.c#L270 I think these are per process.
There was a problem hiding this comment.
I've opened an issue open-telemetry/semantic-conventions#1275
...ntelemetry/instrumentation/runtimemetrics/java8/internal/ExperimentalFileDescriptorTest.java
Outdated
Show resolved
Hide resolved
...ntelemetry/instrumentation/runtimemetrics/java8/internal/ExperimentalFileDescriptorTest.java
Outdated
Show resolved
Hide resolved
|
@xiangtianyu are you able to still take a look at this? Thanks! |
Nobody response my issue open-telemetry/semantic-conventions#1275 . Is there any update about the metrics specification? |
related to #9340 Add file descriptor metrics to agent (support openj9)