Skip to content

Stop logging request.params as json.extra

Igor requested to merge json-extra-cardinality into master

What does this MR do?

This patch aims to avoid field explosion in our rails logs.

Our rails logs have suffered from having too many fields for a while. This produces errors when querying in kibana (https://gitlab.com/gitlab-com/gl-infra/infrastructure/-/issues/9818). It also contributes to ES instability.

In an effort to cut down on the number of fields in our logs, we're looking specifically at places where fields are dynamically created.

One other example of this was !31910 (merged).

This MR is another. :)

This flamegraph shows that 80% of the fields in the rails index are from json.extra:

fields.svg

My hypothesis is that this log line is producing a large portion of those fields. So by being stricter about what is logged, we can cut down on the number of fields.

Removing all of params may seem like a drastic approach, but we need to be mindful of our log schema. We need to be explicit about the fields that we log. Otherwise we have no chance to fix the issues once those logs arrive in ES.

The only consumer of this API is lib/api/api.rb, for the catch-all exception handler:

    rescue_from :all do |exception|
      handle_api_exception(exception)
    end

    ...

    rescue_from Rack::Timeout::RequestTimeoutException do |exception|
      handle_api_exception(exception)
    end

If we can identify specific fields from request.params that we absolutely need to retain, we should definitely add them.

Screenshots

n/a

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

cc @engwan

Edited by Igor

Merge request reports

Loading