prometheus / jmx_exporter

A process for exposing JMX Beans via HTTP for Prometheus consumption
Apache License 2.0
3.06k stars 1.2k forks source link

Fix Exceptions are not handled on premain #873

Closed guhanjie closed 1 year ago

guhanjie commented 1 year ago

Prev code would cause jvm to crash as Exception uncatched in javaagent premain. So this commit make the BindException checked and output definite error and then exit expectedly.

guhanjie commented 1 year ago

@dhoard could you please have a review?

dhoard commented 1 year ago

@guhanjie thanks for the PR! The change looks good.

Since it appears other types of Exceptions are not handled, maybe we should have a generic catch block to catch all possible exceptions, and change the IllegalArgumentException when parsing configuration to a ConfigurationException.

Thoughts?

try {
    // DO STUFF
} catch (Throwable t) {
    synchronized (System.err) {
        System.err.println("Failed to start Prometheus JMX Exporter");
        System.err.println();
        t.printStackTrace();
        System.err.println();
        System.err.println("Prometheus JMX Exporter exiting");
        System.err.flush();
    }
    System.exit(1);
}
guhanjie commented 1 year ago

@guhanjie thanks for the PR! The change looks good.

Since it appears other types of Exceptions are not handled, maybe we should have a generic catch block to catch all possible exceptions, and change the IllegalArgumentException when parsing configuration to a ConfigurationException.

Thoughts?

try {
    // DO STUFF
} catch (Throwable t) {
    synchronized (System.err) {
        System.err.println("Failed to start Prometheus JMX Exporter");
        System.err.println();
        t.printStackTrace();
        System.err.println();
        System.err.println("Prometheus JMX Exporter exiting");
        System.err.flush();
    }
    System.exit(1);
}

It sounds good, I update it, please hava a look again.

guhanjie commented 1 year ago

@dhoard Can you make a new release build and deploy to central mvn repository, because we are suffering such issue, and waiting for new release version CI, rather than local patch. Thanks~

dhoard commented 1 year ago

@guhanjie Can you elaborate on how you are suffering from this issue?

These exceptions should only happen on initial setup/configuration.

guhanjie commented 1 year ago

@guhanjie Can you elaborate on how you are suffering from this issue?

These exceptions should only happen on initial setup/configuration.

We are deploying our service using jmx_exporter java agent, which port may be used on some busy hosts, this would happen on large cluster, incident but crashed and have a heavy core dump file(20-30GB). That's not we expected.

dhoard commented 1 year ago

We are deploying our service using jmx_exporter java agent, which port may be used on some busy hosts, this would happen on large cluster, incident but crashed and have a heavy core dump file(20-30GB). That's not we expected.

You should define an application deployment standard (reserved port) for applications using the exporter.

I don't feel this change is worthy of a point release to work around what I feel is an anti-pattern for application deployment.