RequestLog: Reduce Chance of Memory Leak

[benjchristensen]

Need to evaluate if "hystrix.command.default.requestLog.enabled" should default to false instead of true.

It can potentially cause memory leaks if the request context is not setup right so it may be better for it to be opt-in ... or either:

1. find a way to only use it if we can determine the context is setup right
2. print warning if the size of the log exceeds some large value (1000+ ?)

[benjchristensen]

If using the default RequestContext it will correctly not use RequestLog if not initialized:

https://github.com/Netflix/Hystrix/blob/master/hystrix-core/src/main/java/com/netflix/hystrix/HystrixCommand.java#L845

If someone implements their own ConcurrencyStrategy then it will automatically use the RequestLog.

[benjchristensen]

Instead of changing default behavior a max limit of 1000 has been set for the HystrixRequestLog.

If 1000 commands get added to a log any further will be ignored and warnings output (they will flood logs) to make someone fix the configuration.

logger.warn("RequestLog ignoring command after reaching limit of " + MAX_STORAGE + ". See https://github.com/Netflix/Hystrix/issues/53 for more information.");

Right now it is hardcoded to 1000. If there are actually legit reasons to go higher than that (I and the people sitting near me can't think of a reason as it's generally single-digits or dozens at the high end) we can make it configurable.

RequestLog ignoring command after reaching limit of 1000. See https://github.com/Netflix/Hystrix/issues/53 for more information.

This will limit impact of memory leaks and draw attention to what needs to happen.

The options are:

1. Correctly manage the HystrixRequestContext lifecycle.

https://github.com/Netflix/Hystrix/wiki/How-To-Use#wiki-RequestContextSetup

2. Disable RequestLog

https://github.com/Netflix/Hystrix/wiki/Configuration#wiki-CommandExecution

hystrix.command.default.requestLog.enabled = false

[killme2008]

@benjchristensen Any problems if i disabled request log?

[mattrjacobs]

If you don't need it, then feel free to disable it. We use it fairly heavily for debugging/understanding our system, but YMMV

[killme2008]

@mattrjacobs Thanks for your so quick response.

[freakhill]

In one of our applications we run user provided code querying an eventually consistent database. We use a request context for the request cache (we do not guarantee "read your own writes") so we can end up with a few thousands of commands by batch.

We can fairly easily estimate the amount of requests, is there a way to configure your limit or is it still hardcoded?

[mattrjacobs]

This is still a hardcoded value. Also note that it solely refers to the HystrixRequestLog.

[random-atom]

I'd like to have this warning be made configurable please, while we work over time to analyze what we can change in our code to reduce the number of commands per request. Until then we're having to turn off the request logs which otherwise has valuable information.

Any kind of configuration is fine: the value of MAX_STORAGE, turning it on or off, and/or (if needed) writing the warning out once per request instead of for every command.

Thanks!

[mattrjacobs]

The intent of this restriction is to avoid memory leaks when the request context is not set up properly. IMO, disabling this restriction is a bad idea.

From your description, it sounds like it's possibly expected to have more than 1000 commands in a request. I'd be interested in what that system looks iike

[random-atom]

Sorry for the late reply. Disabling the restriction was just one option. Any of the other options is also fine (a configurable value of MAX_STORAGE, or an option to write the warning out once per request instead of for every command invocation).

Regarding the system, we've used Hystrix to encapsulate things like JMS sends and memcached calls. Combined, these can number in the thousands for certain kinds of heavy requests.

[mattrjacobs]

I'm OK with either of the approaches you mentioned:

1. make this value configurable
2. write out the warning once per request.

I have fairly low bandwidth to work on this presently. How high is the priority on this? If it's high, then submitting a PR would greatly help in getting it into the library.

[cartersm]

Resurrecting this thread to throw my hat in the ring:
We have some pretty heavy requests to our datastore (largely because of small batching to reduce our own memory usage) that can easily number >1000 requests in a single Hystrix request context. Unfortunately, because of the RequestLog ignoring the commands, I can't get visibility into the ~40% of those requests that I can't account for. I'd like to be able to configure MAX_STORAGE (possibly on a per-context basis) so we can (1) staunch the flood of warnings and (2) actually determine what those requests are, as I'm not seeing a good reason for them in our own code.

[DidierLoiseau]

In one of our applications we run user provided code querying an eventually consistent database. We use a request context for the request cache (we do not guarantee "read your own writes") so we can end up with a few thousands of commands by batch.

We are in a similar situation where our code depends on user-provided rules configuration and also involves some calls triggered from class proxies. Our approach to keep it performant was simply to rely on Hystrix's @CacheResult which avoids performing the same call multiple times.

We now noticed that we have this warning, and after adding some logging of HystrixRequestLog.getExecutedCommandsAsString(), it appears that we typically have a log with ~990 calls with RESPONSE_FROM_CACHE.

Moreover it seems we rarely have much more than 1000 calls as we don't see this line repeated a lot.

It would thus be nice if we could at least configure the limit, but also if it was logged only once. Ideally cached results should be aggregated to avoid counting towards this limit.

I know Hystrix is in maintenance mode but I wanted to post this for future reference.