Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is there a way to automatically pass thread static information to the Hystrix Command thread pool? #92

Closed
pparth opened this issue Jan 29, 2013 · 25 comments · Fixed by #711

Comments

@pparth
Copy link

pparth commented Jan 29, 2013

Hello,

I have a number of 3 REST-based micro services. Each of these belongs to a logical layer. A Web app calls the service on the upper layer and the call goes down to the second layer and finally the third one where data are obtained and get responded all the way back. I need to log a common RequestId (as a GUID) that identifies a single web request in all layers. This request id is created on the first service and as a Header it automatically passes from each http client to the next service and from the service to the next client using Client & Servlet filters and a Logback MDC instance. So, in each layer, if the Servlet filter does not get a RequestId header, it creates it, and it puts it in the MDC for the current thread. Whenever an http client request is made, the Client Filter, reads the request id from the MDC, creates a header and passes it to the request and the chain goes on.

If i use a Hystrix Command as a wrapper to the actual http client call, the chain breaks. Apparently, the threads created by a Hystrix Command are not "children" of the thread that called its constructor and therefore the MDC values do not propagate.

Is there a solution to this problem? I read about the execution hooks. Can they offer a solution?

More info at http://logback.qos.ch/manual/mdc.html#autoMDC and http://logback.qos.ch/manual/mdc.html#managedThreads

@benjchristensen
Copy link
Contributor

We do exactly this type of use case at Netflix and use the HystrixConcurrencyStrategy to accomplish it.

The ConcurrencyStrategy and ExecutionHook plugins overlap somewhat in their ability to handle this use case depending on how you want to do it.

The wrapCallable (http://netflix.github.com/Hystrix/javadoc/com/netflix/hystrix/strategy/concurrency/HystrixConcurrencyStrategy.html#wrapCallable(java.util.concurrent.Callable)) method is probably the one you should look at first.

public <T> java.util.concurrent.Callable<T> wrapCallable(java.util.concurrent.Callable<T> callable)

Provides an opportunity to wrap/decorate a Callable<T> before execution.
This can be used to inject additional behavior such as copying of thread state (such as ThreadLocal).

For example, the internal Netflix plugin implementation does this:

    @Override
    public <K> Callable<K> wrapCallable(Callable<K> c) {
        return new ContextCallable<K>(c);
    }

The ContextCallable copies thread state from the calling thread onto the child thread and then cleans up as that thread finishes.

More advanced behavior can be done by implementing getRequestVariable

    @Override
    public <K> HystrixRequestVariable<K> getRequestVariable(HystrixRequestVariableLifecycle<K> rv) {
        return new NetflixRequestVariable<K>(rv);
    }

In this case Netflix overrides the default Hystrix functionality since we already have a "request variable" and prefer to use it so decorate the one we have with the Hystrix interface.

Unless you already have this type of behavior don't bother with that and just stick to wrapCallable.

Your implementation of a wrapping callable would be similar to this: https://github.com/Netflix/Hystrix/blob/master/hystrix-core/src/main/java/com/netflix/hystrix/strategy/concurrency/HystrixContextCallable.java

The difference is that you'll copy whatever context you have:

public class YourCallableWrapper<K> implements Callable<K> {

    private final Callable<K> actual;
    private final SomeContext parentThreadState;

    public YourCallableWrapper(Callable<K> actual) {
        this.actual = actual;
        // copy whatever state such as MDC
        this.parentThreadState = SomeContext.getContextForCurrentThread();
    }

    @Override
    public K call() throws Exception {
        SomeContext existingState = SomeContext.getContextForCurrentThread();
        try {
            // set the state of this thread to that of its parent
            // this is where the MDC state and other ThreadLocal values can be set
            SomeContext.setContextOnCurrentThread(parentThreadState);
            // execute actual Callable with the state of the parent
            return actual.call();
        } finally {
            // restore this thread back to its original state
            SomeContext.setContextOnCurrentThread(existingState);
        }
    }

}

Then you'd implement `wrapCallable' as:

    @Override
    public <K> Callable<K> wrapCallable(Callable<K> c) {
        return new YourCallableWrapper<K>(c);
    }

@benjchristensen
Copy link
Contributor

Hi @pparth,

Did the information above help or do you still have further questions on how to do this?

@pparth
Copy link
Author

pparth commented Feb 4, 2013

Hello,

Thank you for all your interest!

I used my version of a HystrixConcurrentStrategy where i override the wrapCallable method that provides my implementation of a Callable as in your example. I registered my strategy using:

HystrixPlugins.getInstance().registerConcurrencyStrategy(new O2HystrixConcurrencyStrategy());

Callable:

public class O2HystrixContextCallable implements Callable {

private final Callable<K> actual;
private final Map parentMDC;

public O2HystrixContextCallable(Callable<K> actual) {
    this.actual = actual;
    this.parentMDC = MDC.getCopyOfContextMap();
}

@Override
public K call() throws Exception {
    Map childMDC = MDC.getCopyOfContextMap();
    try {
        MDC.setContextMap(parentMDC);
        return actual.call();
    } finally {
        MDC.setContextMap(childMDC);
    }
}

}

Strategy:

public class O2HystrixConcurrencyStrategy extends HystrixConcurrencyStrategy {
@OverRide
public Callable wrapCallable(Callable callable) {
return new O2HystrixContextCallable(callable);
}
}

Nevertheless, when i try to run it, i get the exception that says:

HystrixRequestContext.initializeContext() must be called at the beginning of each request before RequestVariable functionality can be used.

which is odd, as i didn't enable caching anywhere...

@benjchristensen
Copy link
Contributor

Yeah ... that's due to me "cheating" when I know the default implementation is being used since most people won't implement a custom concurrency strategy (at least I don't think so ... but could be wrong).

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

You can turn off the RequestLog as well to avoid this if you want. It was a discussion point about enabling/disabling that functionality by default: #53

I imagine you've already seen this for how to initialize the context: https://github.com/Netflix/Hystrix/wiki/How-To-Use#wiki-RequestContextSetup

What are your thoughts on what should be the default behavior?

@pparth
Copy link
Author

pparth commented Feb 5, 2013

I initialized the context in a servlet filter, which seems to be the perfect place to do so, and everything worked like a charm!
Configuration option to disable RequestLog would do the trick too, so no particular meaning in changing the default behavior.

Not so sure if most people will not have thread info propagation issues though. Unless we are talking about the simplest of applications (and what is the need to use Hystrix for these in the first place, probably when the real benefit of using it, is never clearly understood...), someone will eventually like to pass information from the request thread to the Hystrix pool.

@benjchristensen
Copy link
Contributor

You're probably right ... so perhaps I should add more information around this subject.

Do you mind if I use a slightly modified version of your code copying the MDC state across threads on the wiki?

@pparth
Copy link
Author

pparth commented Feb 5, 2013

Of course! Go ahead! Happy to be of little help!

@eirslett
Copy link
Contributor

eirslett commented Mar 2, 2015

Is it possible to configure what the default strategy should be in a properties file, instead of doing it at the code level?

hystrix.defaultconcurrencystrategy.properties:
hystrix.defaultconcurrencystrategy=com.example.my.custom.Strategy

That way, we could bundle the configuration in an internal/company-level commons library on top of Hystrix.

@benjchristensen
Copy link
Contributor

Archaius is the library used to do configuration and it supports properties files: https://github.com/Netflix/archaius

Hystrix itself delegates all configuration to Archaius and/or the plugins that can be implemented. 

-- 
Ben Christensen - Netflix Edge Engineering
+1.310.782.5511  @benjchristensen

On March 2, 2015 at 2:49:30 AM, Eirik Sletteberg (notifications@github.com) wrote:

Is it possible to configure what the default strategy should be in a properties file, instead of doing it at the code level?

hystrix.defaultconcurrencystrategy.properties:
hystrix.defaultconcurrencystrategy=com.example.my.custom.Strategy

That way, we could bundle the configuration in an internal/company-level commons library on top of Hystrix.


Reply to this email directly or view it on GitHub.

eirslett pushed a commit to eirslett/Hystrix that referenced this issue Mar 10, 2015
Currently, Hystrix lets you define custom strategies in two ways:
1) with System properties
2) with bootstrapping via HystrixPlugins.getInstance().registerXXX
If neither is specified, the default strategy is used.

This change replaces hardwired System.getProperty calls with lookup
via Archaius. So one can override the plugin strategies used with
any configuration provider Archaius is configured with.

Most importantly, it has the effect of checking any file called
"config.properties" on the classpath as well as using system properties.
This lets you configure plugin strategies without having to write
Java code for it, or having to run the application with additional
system properties.

Fixes Netflix#92
@pbetkier
Copy link

Hello,

I've just run into the same issues when implementing my custom HystrixConcurrencyStrategy – I need to wrap callables so that they'd transfer a ThreadLocal variable. The customization is not related to request context and, since what I'm preparing is a library, I'd rather not control the request log options nor register filters for initializing request context for my clients.

Perhaps we can think of an idea to make customizations to HystrixConcurrencyStrategy possible when they are not related to the use of request context. Why do we support using request context optionally when the default strategy is used and enforce context initialization for custom strategies?

@benjchristensen
Copy link
Contributor

Why do we support using request context optionally when the default strategy is used and enforce context initialization for custom strategies?

The key reason it is optional in the default mode is to make it easy to get started with Hystrix when none of the concurrency strategy or request context behaviors are being used. As soon as any of those features get touched then the lifecycle is required. This decision was made primarily out of simplicity, and because Hystrix can not know if a HystrixConcurrencyStrategy needs a RequestContext lifecycle or not, so defaults to requiring it to ensure functional correctness.

If you can propose changes that are safe, understandable, deterministic, and backward compatible for allowing a HystrixConcurrencyStrategy to be implemented without needing a RequestContext lifecycle, then we will review and consider merging. Expect though that it will likely take some time to review that type of change and feel comfortable with it.

@pbetkier
Copy link

Thanks for info. I'll get back if I come up with some solution :)

@pbetkier
Copy link

I don't have any changes to the codebase to propose, though I'd like to share my HystrixConcurrencyStrategy implementation, maybe someone will find it useful. Because I know MyCallable doesn't require RequestContext lifecycle, this implementation preserves behaviour of the default HystrixConcurrencyStrategy in terms of neither requiring request context to be initialized nor request log to be disabled.

public class MyHystrixConcurrencyStrategy extends HystrixConcurrencyStrategy {

    @Override
    public <T> Callable<T> wrapCallable(Callable<T> callable) {
        return new MyCallable<>(callable);
    }

    @Override
    public <T> HystrixRequestVariable<T> getRequestVariable(HystrixRequestVariableLifecycle<T> rv) {
        return new HystrixRequestVariableDefault<T>() {
            @Override
            public T get() {
                if (!HystrixRequestContext.isCurrentThreadInitialized()) {
                    return null;
                }
                return super.get();
            }
        };
    }
}

@mwhipple
Copy link

the code from @pbetkier above doesn't seem to support RequestContext properly when it is initialized and does not provide the behavior of the default strategy: https://github.com/Netflix/Hystrix/blob/master/hystrix-core/src/main/java/com/netflix/hystrix/strategy/concurrency/HystrixConcurrencyStrategy.java#L147
The initializeValue and shutdown methods should also be provided in addition to the get above.

The get override is needed to provide the same functionality as offered by https://github.com/Netflix/Hystrix/blob/master/hystrix-core/src/main/java/com/netflix/hystrix/AbstractCommand.java#L265.

This logic could potentially be reworked to provide a defined implementation of HystrixRequestVariable which contains that logic and is used by the default concurrency strategy to shift that decision out of the client code and provide greater consistency and modularity (while maintaining an explicit opt-out for custom strategies). I could put a PR together if there's interest in that approach. @benjchristensen?

@mattrjacobs
Copy link
Contributor

@mwhipple Yeah, if you're able to submit a PR, I'd be happy to review and get opinions from others in this thread.

@mattrjacobs mattrjacobs reopened this Oct 16, 2015
@spencergibb
Copy link
Contributor

I'm interested in this as well. We are looking to pass tracing information through to Hystrix commands on separate threads.

@mattrjacobs
Copy link
Contributor

@spencergibb To clarify, the capability already exists to pass contextual data from caller threads to Hystrix threads and is described above (#92 (comment)). The work being discussed here will (potentially) provide a way to write this logic in a way that's more decoupled from the HystrixRequestContext than it is today.

@spencergibb
Copy link
Contributor

Thanks for the clarification @mattrjacobs. I think I'm still interested :-), but might be able to get along with the more coupled way.

@Dreampie
Copy link

if use threadlocal to set some value,hystrix will lost it, how to resolve?

@mattrjacobs
Copy link
Contributor

@Dreampie You can use a concurrency strategy as described on the Wiki

@ulmermark
Copy link

I am following this approach to forward a request header throughout the microservice call chain....all the above makes sense, but I can not see how to "access" the HystrixRequestVariable that is injected into the HystrixRequestContext in my servlet filter..
` public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException {

    if (!(request instanceof HttpServletRequest) || !(response instanceof HttpServletResponse)) {
        throw new ServletException("HystrixRequestContextServletFilter just supports HTTP requests");
    }
    HttpServletRequest httpRequest = (HttpServletRequest) request;

    HystrixRequestContext context = HystrixRequestContext.initializeContext();

    try {
        HystrixRequestVariableDefault<String> xAuthToken = new HystrixRequestVariableDefault<String>();
        xAuthToken.set(httpRequest.getHeader("x-auth-token"));
        chain.doFilter(request, response);
    } finally {
        if ( null != context) context.shutdown();
    }
}

`
I would like to access this value at a later time in a Feign Client, and inject the value into a request header for use in another microservice.

I am currently using a custom RequestInterceptor for my Feign client as shown:

'
@bean
public RequestInterceptor getRequestInterceptor(){

     final String xAuthTokenHeaderName = "x-auth-token";

     return new RequestInterceptor(){
        @Override
        public void apply(RequestTemplate template) {

            HystrixRequestContext hystrixContext = HystrixRequestContext.getContextForCurrentThread();
            HystrixConcurrencyStrategy concurrencyStrategy = HystrixPlugins.getInstance().getConcurrencyStrategy();
            // TODO need to access hystrixrequestvariable in the current hystrix context...how is this done.


            if (!Strings.isNullOrEmpty(pemsActor))template.header("Actor", pemsActor);
            if ( !Strings.isNullOrEmpty(pemsUser))template.header("User", pemsUser);
            if ( !Strings.isNullOrEmpty("x-auth-token"))template.header(xAuthTokenHeaderName, "x-auth-token goes here");
        }
     };

'
Looked at many examples, followed the code for the debugger...but it is not obvious on how to access the HystrixRequestVariableDefault insyance injected into the HystrixRequestContext in the servlet filter.

@mattrjacobs
Copy link
Contributor

The concept of variables which span an entire request is pretty funky, though quite useful. The only way to manage this is to use a static object which you can then call .get() on in multiple places.

In this case, you could create a new XAuthToken class with static .get()/set() methods. That class would wrap a HystrixRequestVariable<String>, and forward the .get()/set() calls to the HystrixRequestVariable. Then your servlet filter could call .set(), and any other class could use .get() to get the current value for the current request.

@ulmermark
Copy link

Thanks..makes sense...hence all the "...Holder" classes.

@jiacai2050
Copy link

For anyone using hystrix-clojure, I create a simple library to deal with dynamic var binding in hystrix command, it's the same issue with threadlocal.

@utkarsh578
Copy link

Hi All, i had HystrixEventNotifier, and HystrixConcurrencyStrategy plugins registered.
So when TIMEOUT happens, then the HystrixEventNotifier is executed in which there is no mdc/thread local context but after that is falls on fallback which is having all MDC context and surprisingly both happens in the same Timer thread. My use case is to get the MDC context at HystrixEventNotifier. Any idea anyone ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.