Skip to content
This repository

Make RequestId globally available #5176

Closed
mperham opened this Issue February 25, 2012 · 16 comments

6 participants

Mike Perham Aaron Patterson Marcos Toledo Tim Carey-Smith Adam Hawkins José Valim
Mike Perham

RequestId is only available via request.uuid at the moment. This should be attached to a Thread local so that the uuid can be accessed in ActiveRecord and other libraries that have nothing to do with HTTP. In my case, I want Sidekiq to include the RequestId with any messages so the Sidekiq log can show the RequestId that created the message but I don't want to tack on another optional parameter to my API and have the user to pass me the request.uuid, like so:

def create
  # create user
  # send a delayed email
  UserEmails.delay(:uuid => request.uuid).send_welcome_email(@user)
  # run something async
  SomeWorker.context(:uuid => request.uuid).perform_asyc(@user)
end

That's messy. It would be nice if I could just internally call ActionDispatch::Request.uuid to get the current uuid or nil.

Mike Perham mperham referenced this issue in mperham/sidekiq February 25, 2012
Closed

Support Rails 3.2's RequestId feature #56

Aaron Patterson
Owner

I don't agree with this. The uuid is data that's associated with the request. Setting it as (essentially) a global variable on some class seems bad. What if the user wants some other unique id logged along with Sidekiq? Would Rails have to provide a setter so that Sidekiq will get the right value?

The coupling seems bad. Requiring users to call some method that has nothing to do with Sidekiq in order to get a value logged to Sidekiq seems bad.

Mike Perham

Well, I think of the request ID as a global transaction ID, as in "This request kicked off all this work in these other subsystems". See log_weasel as an example implementation of what I'm talking about. I'm happy to defer on impl details but getting a global transaction ID is what I'm looking for.

Marcos Toledo

I see where @mperham is coming from. ActionDispatch::RequestId describes its use as "The unique request id can be used to trace a request end-to-end and would typically end up being part of log files from multiple pieces of the stack.", and I agree that this use is largely hindered by the fact that you basically have to pass the request id around to all those end to end pieces as opposed to just reading it from a thread local.

On the other hand, I have to confess I find it something that rails shouldn't even provide, and that sidekiq should just add support for log_weasel instead (if it doesn't already support it through resque's callbacks) and that someone interested in this functionality should probably use log_weasel or a similar gem instead.

Mike Perham

@mtoledo log_weasel is by no means a popular or even well known gem, the only reason I've heard of it is because I worked at Carbon Five. The only way this will become standard is if a popular framework like Rails or Sinatra integrates it.

Marcos Toledo

Yeah, well I still think that a generic middleware that would support it in a consistent way across multiple rack frameworks and subsystems is probably better than having it built in into rails. Although I agree it'd have to be hooked up in rails in order to be popular, I'm not convinced its necessarily a practice worth the trade offs to be made popular, specially when you can just install a gem.

I still agree with you though that having request_id available on a thread local would make it way more convenient, and that for the uses described on its documentation, the way it works right now is of very limited utility. Therefore, since rails already has a request_id, maybe it should go for it. I'll let someone else weigh in.

Tim Carey-Smith

Would it make sense to only set the Thread-local if config.allow_concurrency == false?
Having a Thread-unsafe value added by default is a little concerning.

It reminds me of the plugin which added User.current, convenient but unsafe.

Marcos Toledo

I don't think it really makes any difference whether we're running one or multiple requests per process. A thread local is ofc thread safe by definition, so its really not an issue about thread safety.

Its about binding values in globals for convenient uses in other layers your app, and whether cohesion is a worth tradeoff for this convenience. I think adding a thread local to access request_id is somewhat similar to adding the request to a thread local in a cohesion point of view, so say your db could store the ip address that made the request that eventually caused that record to be stored. Convenient, yes, but globals make everything more convenient, and we've got to be mindful of their uses.

Which doesn't mean its always wrong to use a global or a thread local. That's why I said I'd love to hear someone else's opinion, and maybe having request_id on a thread local is actually a legitimate use.

Tim Carey-Smith

I'm was actually talking about multiple requests per thread.
Which makes my suggestion not workable anyway.

The way I share request information with the model layer or anything lower than the Rails controller or view, is to pass the request information down.
Using global structures is convenient I must agree, but introduces coupling between systems which is not immediately apparent.

My concern is when this feature is added to Rails and gems like Sidekiq start depending on it in such a way which does not allow a user like myself who is making multiple requests per thread to use Sidekiq and depend on the request id (in this case) being consistent from within Sidekiq.

Mike Perham

If you are making multiple concurrent requests per thread without Fibers, you aren't using Rails. Thread-locals work with Fibers.

Marcos Toledo

Agreed, I don't think there's any thread safety or compatibility issue involved here. It's mainly a cohesion discussion. And like I said request_id as and end to end resource without being a thread local makes it kinda hard to use. The more I think about this the more I think its legit tbh.

Tim Carey-Smith

I wasn't aware Rails was not attempting to support my use case, though I'd prefer to be using Fibers.
If that is the case, I am in agreement that it is a just a coupling question.

In my opinion, a Thread-local with Fibers is not really a global structure, as it is context specific.

Adam Hawkins

@mperham do you have an example of how you would use it? Rails.request_id? Just floating that.

Mike Perham

@twinturbo I guess I could have the user set an around_filter like like around_filter { Sidekiq.current_request_id = request.uuid; yield } but I was hoping for a zero config solution.

Marcos Toledo

Is there any chance we can revisit this now that we're planning to have a background job queue api on rails 4? Maybe now the api can have a setter for the request id which the framework sets automatically during the request cycle, given its no longer something sidekiq specific.

José Valim
Owner

I am also :-1: for exactly the same arguments given by @tenderlove. For one, we store the I18n locale in a thread local and I have already tried to get rid of that many times. It is a bad coupling with the request and I would prefer if it was passed down properly.

Aaron Patterson tenderlove closed this April 30, 2012
Aaron Patterson
Owner

This should just be done in a middleware or something and associated with the request. Having another class attribute is bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.