Completely disable thread pool initialization for metrics feature #731
Comments
seratch
added
project:slack-api-client
|
Hi @dsultanr, thanks for writing in! Although the daemon thread pools for metrics work without any issues with OpenJDK etc, the part of this library might not be compatible with the runtime you use. As a workaround, can you try a custom package hello;
import com.slack.api.Slack;
import com.slack.api.SlackConfig;
import com.slack.api.audit.AuditConfig;
import com.slack.api.methods.MethodsConfig;
import com.slack.api.methods.response.api.ApiTestResponse;
import com.slack.api.scim.SCIMConfig;
public class Example {
public static void main(String[] args) throws Exception {
// You can reuse this config for all client instantiation
SlackConfig config = new SlackConfig();
config.setMethodsConfig(MethodsConfig.builder().statsEnabled(false).metricsDatastore(null).build());
config.setSCIMConfig(SCIMConfig.builder().statsEnabled(false).metricsDatastore(null).build());
config.setAuditConfig(AuditConfig.builder().statsEnabled(false).metricsDatastore(null).build());
Slack slack = Slack.getInstance(config);
ApiTestResponse response = slack.methods().apiTest(r -> r.foo("bar"));
System.out.println(response);
}
}Also, okhttp (the underlying HTTP client of this library) generates its thread pool under the hood. To smoothly clean it up, I recommend calling I hope this was helpful to you. |
|
Hi @seratch, thanks for quick reply! I added a config you proposed but SDK still creates the threads for metrics here is the log: |
seratch
added
enhancement
|
@dsultanr Thanks for sharing the result! hmm, this SDK should provide a new option to completely disable the metrics feature. I cannot tell when we can make the release yet but I will update you once the fix is available. As a workaround, please consider using a different HTTP client for performing webhook requests and Web API calls for now. |
seratch
changed the title
|
After working on this task a bit today, I found that supporting this with full backward compatibility may take a bit longer than I expect (it's feasible but not so simple). Let me move this to v2 milestone. |
|
@seratch do I understand correctly that the problem is with those threads not properly closed? And so does it mean that Slack#close() would only close okhttp threads, but not the metrics feature threads? As you mentioned you would add an option to disable metrics, but why not add the proper closing within Slack#close(), so that it would close everything and I wouldn't need to disable anything. I may have misunderstood something completely, so my apologies if that is the case. My situation is that I'm invoking Slack#close() upon shutdown (tomcat shutdown), but I still get |
|
Hi @metaruslan, thanks for your comment! I do understand many people may use Slack / MethodsClient as a singleton object but the metrics database can be shared among multiple Slack instances. Thus, closing the threads for the metrics as well in Slack#close() is not an appropriate solution here. |
|
@seratch thanks a lot for the answer! Still not 100% clear to me. Even if I have multiple Slack instances I would still have an event of the JVM shutdown, so maybe there could be a point to configure a shutdown hook or something like that. I guess my problem is that I lack the knowledge of the overall design here :-) My team is switching from simple webhook calls to using the Slack SDK. The slack SDK does look good and a pleasure to use, but it feels a bit heavy given all the threads it creates :-( As far as I understood the problem will go away in version 2. Is there an ETA for version 2? |
No, there isn't and the version 2 won't come soon. As the change for this new option does not bring any breaking changes to existing apps, we can have that change in 1.x series. The reason why we have this task in v2 milestone is just due to our priorities in the short term. We may revisit this in the future but not in a few weeks. |
|
Hi there, I haven't verified if this works for the situations described here yet but setting a new option If anyone tries it out and see the option works well, please share the results here 👋 |
|
Thanks. I'm afraid we have no option to test it right now
|
|
I'm not 100% sure I understand the question, but one might be able to disable the threads by calling setStatsEnabled(false) on the default MetricsDatastore objects. listOf(
MethodsConfig.DEFAULT_SINGLETON.metricsDatastore,
AuditConfig.DEFAULT_SINGLETON.metricsDatastore,
SCIMConfig.DEFAULT_SINGLETON.metricsDatastore,
Slack.getInstance().config.methodsConfig.metricsDatastore,
Slack.getInstance().config.auditConfig.metricsDatastore,
Slack.getInstance().config.scimConfig.metricsDatastore
).forEach { it.isStatsEnabled = false }
// if there is an app config
val appConfig = AppConfig()
listOf(
appConfig.slack.config.methodsConfig.metricsDatastore,
appConfig.slack.config.auditConfig.metricsDatastore,
appConfig.slack.config.scimConfig.metricsDatastore
).forEach { it.isStatsEnabled = false } |
|
Apparently all configs + on every initialization of a new one, stats thread are created. What worked for me is: |
I have a strange issue in a Lotus Domino with the Slack api
The Slack SDK version
Java Runtime version
[010392:000002-00007FF303406740] 04/21/2021 10:00:57 Java runtime version: 8.0.5.21 - pxa6480sr5fp21-20180830_01(SR5 FP21)
[010392:000002-00007FF303406740] 04/21/2021 10:00:57 JVM version: JRE 1.8.0 Linux amd64-64-Bit 20180829_395745 (JIT enabled, AOT enabled) OpenJ9 - e82188c OMR - eeaa30e IBM - 98805ca
OS info
Linux 46f0066d17e5 3.10.0-957.10.1.el7.x86_64 #1 SMP Mon Mar 18 15:06:45 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Steps to reproduce:
Expected result:
clean exit
Actual result:
threads cleanup error, logs are in top of this message
error thrown even if I comment out block with slack.send call and just leave Slack.getInstance()
Did threads debug (debugger code here) before agent exit and here is the output:
The text was updated successfully, but these errors were encountered: