Slowlog loggers do not seem to be cleaned up when no longer needed #56171
Comments
|
Pinging @elastic/es-core-infra (:Core/Infra/Logging) |
|
The underlying problem here is that Log4j doesn't do any cleanup of loggers, once a logger is created, it is forever registered in the logger registry and there is no cleanup. This is why we introduced
I'm not sure that I see the benefits of per-index slow log settings. If I want to log what is slow, I would want that for all of my indices. Therefore, I advocate that we remove per-index slow log settings. |
|
There is a https://logging.apache.org/log4j/2.x/log4j-api/apidocs/org/apache/logging/log4j/spi/LoggerRegistry.WeakMapFactory.html that possibly could help us. |
|
The weak map factory is designed for web servers like Tomcat where there are servlets that carry various context used when creating the logger. Using the weak map factory in Elasticsearch means replacing the logger registry which means replacing the logger context (since it holds a private reference to the logger registry and can not be controlled) which means replacing the logger context factory. It's a lot of hoops to jump through and we end up copying a lot of Log4j behavior. Again, I question the value of per-index slow log settings to begin with. Let's start with what features we want, and then engineer the solution. |
|
we discussed this during core-infra sync on 6th May. The conclusion is that we will deprecate the use of changing log level per index in 7.x and remove it in 8. We will still allow changing thresholds per index. To fix a memory leak in 7.x we will revert the change that creates a new log4j logger per index and will do a "manual" check of index name/package checks. In ES8 we will no longer allow changing log level per index. That means The option of maintaining current behaviour was rejected. It would require to maintain too many classes from log4j2. |
Slow loggers should use single shared logger as otherwise when index is deleted the log4j logger will remain reachable (log4j is caching) and will create a memory leak. closes #56171
Slow loggers should use single shared logger as otherwise when index is deleted the log4j logger will remain reachable (log4j is caching) and will create a memory leak. closes elastic#56171
Slow loggers should use single shared logger as otherwise when index is deleted the log4j logger will remain reachable (log4j is caching) and will create a memory leak. closes elastic#56171
|
closed by #56708 |
Slow loggers should use single shared logger as otherwise when index is deleted the log4j logger will remain reachable (log4j is caching) and will create a memory leak. closes elastic#56171
Slow loggers should use single shared logger as otherwise when index is deleted the log4j logger will remain reachable (log4j is caching) and will create a memory leak. closes elastic#56171
Setting a slow log level requires an unnecessary conditional logic in SearchSlowLog and IndexingSlowLog The behaviour of setting a level on a slow logger can be achieved with correct slow log threshold settings. This PR is removing slow log and modifies tests to achieve the same behaviour with changing threshold. relates #56171
Investigating https://discuss.elastic.co/t/index-creation-slows-down-over-time/230775 I wrote a script that creates and deletes indices repeatedly and left it running for several hours against a 7.6.2 cluster. Each iteration had indeed slowed down as reported, from around
6sto around10s.The hot threads API often reported a substantial amount of time spent doing things with loggers:
I took a heap dump and observed almost 150,000 loggers of the forms
index.indexing.slowlog.<INDEX_UUID>,index.search.slowlog.fetch.<INDEX_UUID>andindex.search.slowlog.query.<INDEX_UUID>(equal parts of each). There were only 1280 other instances oforg.apache.logging.log4j.core.Logger.I suspect this is a consequence of #47234 which introduced per-index loggers.
The text was updated successfully, but these errors were encountered: