[sc] Don't send sc. WARNING if too many metrics

[elafarge]

We were sending a service check warning for integrations with too many
metrics which is not the behaviour we'd like to have. Even though there
are too many metrics, the can_connect service check should be green.

[olivielpeau]

Tell me if I'm missing something, but I think all we need to do is to remove this line: https://github.com/DataDog/jmxfetch/blob/master/src/main/java/org/datadog/jmxfetch/App.java#L199

What do you think? 😃

[elafarge]

Indeed, that would have been faster but...

1. I don't like to have more lines removed than lines added in my Github statistics
2. Actually if we do that, we won't get the warning on the datadog-agent info page OR we still wan't to have that one. This they are bound to the same variable at this point in the code I'm afraid it's not possible to just remove that line whilst keeping the WARNING on the info page. Correct me if I'm wrong :)

Oh and thanks a lot for your review btw. :)

[remh]

@elafarge

"When debugging, novices insert corrective code; experts remove defective code."
Richard Pattis

:) sometimes removing code is better.

But yeah i agree, we still want the warning to be displayed in the info page (and sent as other agent check warnings in the infra overview if possible).

[elafarge]

@yannmh @remh I decoupled the service checks and status. I didn't add a second service check for the alert on the number of metrics but that's something I could do if it appears to be relevant :)

[yannmh]

Can we simplify (factorize) this block with something like :

while (it.hasNext()) {
    String instanceStatus = Status.STATUS_OK;
    String serviceCheckStatus = Status.STATUS_OK;
    String message = null;

    try {
        // Instantiate instanceStatus, serviceCheckStatus, message
        doSomething();
    } catch (Exception) {
        // Modify instanceStatus, serviceCheckStatus, message
        handleException();
    }

    // Factorize instance status and service check report
    this.reportStatus(appConfig, reporter, instance, metrics.size(), instanceMessage, instanceStatus);
    this.sendServiceCheck(reporter, instance, instanceMessage, scStatus);
}

?

[yannmh]

Thanks for tackling the issue @elafarge.

While I think decoupling service checks and instance status is the right thing to do, I am a bit worried about the resulting code duplication. I added some suggestion to refactorize some code paths 🎨, please let me know what you think

[elafarge]

Thank you so much for you enlightening suggestions @yannmh . I took them into account and updated the PR. Let me know what you think about it ;-)

[yannmh]

Thanks @elafarge !