-
Notifications
You must be signed in to change notification settings - Fork 1k
add file descriptor metrics #11876
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
add file descriptor metrics #11876
Conversation
.../opentelemetry/instrumentation/runtimemetrics/java8/internal/ExperimentalFileDescriptor.java
Outdated
Show resolved
Hide resolved
.../opentelemetry/instrumentation/runtimemetrics/java8/internal/ExperimentalFileDescriptor.java
Outdated
Show resolved
Hide resolved
.../opentelemetry/instrumentation/runtimemetrics/java8/internal/ExperimentalFileDescriptor.java
Outdated
Show resolved
Hide resolved
...va/io/opentelemetry/instrumentation/runtimemetrics/java8/internal/FileDescriptorMethods.java
Outdated
Show resolved
Hide resolved
.../opentelemetry/instrumentation/runtimemetrics/java8/internal/ExperimentalFileDescriptor.java
Outdated
Show resolved
Hide resolved
...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? |
hi @xiangtianyu! that's ok, just add a comment in the code pointing to that semantic convention issue, and we can still merge the PR because it's under "experimental" metrics anyways |
|
I've add comments, please take a look @breedx-splk @trask @laurit |
.../opentelemetry/instrumentation/runtimemetrics/java8/internal/ExperimentalFileDescriptor.java
Outdated
Show resolved
Hide resolved
.../opentelemetry/instrumentation/runtimemetrics/java8/internal/ExperimentalFileDescriptor.java
Outdated
Show resolved
Hide resolved
| if (openFileDescriptorCount != null) { | ||
| observables.add( | ||
| meter | ||
| .upDownCounterBuilder("process.open_file_descriptor.count") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this name isn't exactly the same as process.open_file_descriptors that is used by the host metric receiver we might as well go with a different name. I think it would be best to discuss these names and descriptions at the SIG meeting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, how can i propose this as a topic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on https://github.com/open-telemetry/semantic-conventions/pull/1618/files
as a working group we believe it is important that
processnamespace and runtime namespace metrics remain separate, becauseprocessmetrics are meant to represent an OS-level process as the instrumentation source, whereas runtime metrics represent the language runtime as the instrumentation source.
I'd suggest using jvm.open_file_descriptor.count
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trask do you also have suggestions for the metric descriptions and unit. Currently the unit is {count}.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch: open-telemetry/semantic-conventions#1662
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for description, I think we can use
Number of file descriptors in use by the process.
.../opentelemetry/instrumentation/runtimemetrics/java8/internal/ExperimentalFileDescriptor.java
Outdated
Show resolved
Hide resolved
| if (openFileDescriptorCount != null) { | ||
| observables.add( | ||
| meter | ||
| .upDownCounterBuilder("process.open_file_descriptor.count") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on https://github.com/open-telemetry/semantic-conventions/pull/1618/files
as a working group we believe it is important that
processnamespace and runtime namespace metrics remain separate, becauseprocessmetrics are meant to represent an OS-level process as the instrumentation source, whereas runtime metrics represent the language runtime as the instrumentation source.
I'd suggest using jvm.open_file_descriptor.count
.../opentelemetry/instrumentation/runtimemetrics/java8/internal/ExperimentalFileDescriptor.java
Outdated
Show resolved
Hide resolved
|
it looks like some progress was made in semconv in this area: |
You are right, but it seems that someone has already implemented this metric Line 46 in cb6abd2
|
|
@xiangtianyu should we close this PR then? |
|
This PR has been labeled as stale due to lack of activity and needing author feedback. It will be automatically closed if there is no further activity over the next 7 days. |
related to #9340 Add file descriptor metrics to agent (support openj9)