Skip to content

Don't require lib/gitlab/request_profiler/middleware.rb in config/initializers/request_profiler.rb

What does this MR do?

We should never use require for application files that are in the auto-load paths (note that eager-load paths are included in the auto-load paths) since Rails takes care of auto-loading files, as long as they're named and placed in the correct place.

Also, when referencing a constant, in some cases it needs to be scoped properly to avoid ambiguous references.

For instance in this particular case, by referencing RequestProfiler in Gitlab::RequestProfiler::Middleware#profile?, Rails throws a A copy of Gitlab::RequestProfiler::Middleware has been removed from the module tree but is still active! error. I believe the issue is as follows:

  1. RequestProfiler is expanded to Gitlab::RequestProfiler::Middleware::RequestProfiler and is undefined at this point.

  2. A call to ActiveSupport::Dependencies::ModuleConstMissing#const_missing is issued, which calls ActiveSupport::Dependencies#load_missing_constant.

  3. An exception is raised because of the following code

      unless qualified_const_defined?(from_mod.name) && Inflector.constantize(from_mod.name).equal?(from_mod)
        raise ArgumentError, "A copy of #{from_mod} has been removed from the module tree but is still active!"
      end

It's not clear to me why this error occurs at this point, but what's sure is that replacing RequestProfiler with Gitlab::RequestProfiler in the middleware fixes the issue since it tells Rails that it should look for a Gitlab::RequestProfiler constant instead of looking for Gitlab::RequestProfiler::Middleware::RequestProfiler, thus not triggering the const_missing behavior.

Another solution for this particular case would be to simply get rid of lib/gitlab/request_profiler.rb and put all the logic in lib/gitlab/request_profiler/middleware.rb directly, as suggested in https://gitlab.com/gitlab-org/gitlab-ce/issues/20452#note_20219167.

Are there points in the code the reviewer needs to double check?

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Closes #20452 (closed)

Merge request reports