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

Allow closing observers in oshi library #10358

Closed
manikmagar opened this issue Jan 31, 2024 · 2 comments · Fixed by #10364
Closed

Allow closing observers in oshi library #10358

manikmagar opened this issue Jan 31, 2024 · 2 comments · Fixed by #10364
Labels
contribution welcome Request makes sense, maintainers probably won't have time, contribution would be welcome enhancement New feature or request

Comments

@manikmagar
Copy link
Contributor

manikmagar commented Jan 31, 2024

Is your feature request related to a problem? Please describe.

oshi library helps to capture the Process and System metrics.

All observers are built with a callback but none of the returned AutoCloseables are captured for a clean shutdown at the end.

https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/oshi/library/src/main/java/io/opentelemetry/instrumentation/oshi/ProcessMetrics.java#L23C3-L38C16

public static void registerObservers(OpenTelemetry openTelemetry) {
    Meter meter = openTelemetry.getMeterProvider().get("io.opentelemetry.oshi");
    SystemInfo systemInfo = new SystemInfo();
    OperatingSystem osInfo = systemInfo.getOperatingSystem();
    OSProcess processInfo = osInfo.getProcess(osInfo.getProcessId());

    meter
        .upDownCounterBuilder("runtime.java.memory")
        .setDescription("Runtime Java memory")
        .setUnit("By")
        .buildWithCallback(
            r -> {
              processInfo.updateAttributes();
              r.record(processInfo.getResidentSetSize(), Attributes.of(TYPE_KEY, "rss"));
              r.record(processInfo.getVirtualSize(), Attributes.of(TYPE_KEY, "vms"));
            });
.
.
....
}

Describe the solution you'd like

I would like the registerObserver method to return a list of AutoCloseables instead of void. I am happy to send a PR if the proposed change is okay.

  public static List<AutoCloseable> registerObservers(OpenTelemetry openTelemetry) {
    Meter meter = openTelemetry.getMeterProvider().get("io.opentelemetry.oshi");
    SystemInfo systemInfo = new SystemInfo();
    HardwareAbstractionLayer hal = systemInfo.getHardware();
    
    List<AutoCloseable> observables = new ArrayList<>();
    observables.add(meter
        .upDownCounterBuilder("system.memory.usage")
        .setDescription("System memory usage")
        .setUnit("By")
        .buildWithCallback(
            r -> {
              GlobalMemory mem = hal.getMemory();
              r.record(mem.getTotal() - mem.getAvailable(), ATTRIBUTES_USED);
              r.record(mem.getAvailable(), ATTRIBUTES_FREE);
            }));

    ..... add other observers .....

    return observables;
}

This will then allow any calling classes to manage the cleanup.

Java runtime metrics utilizes this proposed approach.

Describe alternatives you've considered

I can create a custom implementation in my code and re-implement this but I would prefer to use this standard library instead. I am happy to send a PR if the proposed change is okay.

Additional context

No response

@manikmagar manikmagar added enhancement New feature or request needs triage New issue that requires triage labels Jan 31, 2024
@trask
Copy link
Member

trask commented Jan 31, 2024

hi @manikmagar! it looks like the runtime-telemetry module is already using this pattern, so it sounds like a good idea to me

@trask trask added contribution welcome Request makes sense, maintainers probably won't have time, contribution would be welcome and removed needs triage New issue that requires triage labels Jan 31, 2024
@manikmagar
Copy link
Contributor Author

Thank you @trask for the super speedy response :). I will send the PR shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution welcome Request makes sense, maintainers probably won't have time, contribution would be welcome enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants