Commit 9c62fa8f authored by Bob Van Landuyt's avatar Bob Van Landuyt 💬
Browse files

Don't enforce logged keys to be known by Labkit 

It would be handy if all projects used the same keys to log data,
which is why I initially added this list, but having to modify all of
the projects each time would become cumbersome to keep it in sync will
become cumbersome.

So this changes that all keys are allowed: when passing them in
through Labkit, a meta. prefix is added (for logs). These will be
propagated by Labkit when found in Sidekiq jobs.

Now we need to pay attention in the other projects that will push into
a context that they use the same format. But having duplicate
information logged in different fields is likely better than losing
information.
parent 1d201ff6

lib/labkit/context.rb

+1 −5
Original line number Diff line number Diff line
@@ -9,7 +9,6 @@ require "active_support/core_ext/string/inflections" 
module Labkit

  # A context can be used to provide structured information on what resources

  # GitLab is working on within a service.

  # The currently supported keys are defined in the `KNOWN_KEYS` constant.

  #

  # Values can be provided by passing a hash. If one of the values is a Proc

  # the proc will only be called when the value is actually needed.
@@ -28,8 +27,6 @@ module Labkit 
    CORRELATION_ID_KEY = "correlation_id"

    RAW_KEYS = [CORRELATION_ID_KEY].freeze

    HEADER_PREFIX = "X-Gitlab-"

    KNOWN_KEYS = %w[user project root_namespace subscription_plan caller_id

                    remote_ip related_class feature_category client_id].freeze



    class << self

      def with_context(attributes = {})
@@ -122,7 +119,6 @@ module Labkit 


    def assign_attributes(attributes)

      attributes = attributes.transform_keys(&method(:log_key))

      attributes = attributes.slice(*known_log_keys)



      data.merge!(attributes)
@@ -138,7 +134,7 @@ module Labkit 


    private



    delegate :log_key, :known_log_keys, to: :class

    delegate :log_key, to: :class



    attr_reader :data

lib/labkit/middleware/sidekiq/context/server.rb

+1 −0
Original line number Diff line number Diff line
@@ -10,6 +10,7 @@ module Labkit 
          def call(_worker_class, job, _queue)

            worker_name = (job["wrapped"].presence || job["class"]).to_s

            data = job.merge(Labkit::Context.log_key(:caller_id) => worker_name)

                      .select { |key, _| key.start_with?("#{Labkit::Context::LOG_KEY}.") || Labkit::Context::RAW_KEYS.include?(key.to_s) }



            Labkit::Context.with_context(data) do |_context|

              yield

spec/labkit/context_spec.rb

+11 −8
Original line number Diff line number Diff line
@@ -130,13 +130,15 @@ describe Labkit::Context do 
    let(:expected_hash) do

      log_hash(user: "user",

               root_namespace: "namespace",

               project: "project")

               project: "project",

               "random.key": "included")

    end



    it "returns a hash containing the expected values" do

      context = described_class.new(user: "user",

                                    project: "project",

                                    root_namespace: "namespace")

                                    root_namespace: "namespace",

                                    "random.key": "included")



      expect(context.to_h).to include(expected_hash)

    end
@@ -152,6 +154,7 @@ describe Labkit::Context do 
        user: -> { "user" },

        root_namespace: -> { "namespace" },

        project: -> { "project" },

        "random.key": -> { "included" },

      )



      expect(context.to_h).to include(expected_hash)
@@ -185,12 +188,14 @@ describe Labkit::Context do 
        root_namespace: -> { "namespace" },

        project: -> { "project" },

        caller_id: "ProjectsController#show",

        random_thing: "Random",

      )



      expect(context.to_headers).to include("X-Gitlab-Meta-User" => "user",

                                            "X-Gitlab-Meta-Root-Namespace" => "namespace",

                                            "X-Gitlab-Meta-Project" => "project",

                                            "X-Gitlab-Meta-Caller-Id" => "ProjectsController#show")

                                            "X-Gitlab-Meta-Caller-Id" => "ProjectsController#show",

                                            "X-Gitlab-Meta-Random-Thing" => "Random")

    end



    it "does not include empty values" do
@@ -205,7 +210,7 @@ describe Labkit::Context do 
  end



  describe "#initialize" do

    it "assigns only known keys as strings" do

    it "assigns all keys as strings" do

      context = described_class.new(

        # rubocop: disable Style/HashSyntax

        # deliberately testing asigning symbols and strings as keys
@@ -215,8 +220,7 @@ describe Labkit::Context do 
        # rubocop: enable Style/HashSyntax

      )



      expect(data_from(context)).to include(log_hash("user" => "u", "project" => "p"))

      expect(data_from(context).keys).not_to include(described_class.log_key("something_else"))

      expect(data_from(context)).to include(log_hash("user" => "u", "project" => "p", "something_else" => "nothing"))

    end



    it "assigns known keys starting with the log key" do
@@ -224,8 +228,7 @@ describe Labkit::Context do 
        log_hash(project: "p", root_namespace: "n", user: "u", something_else: "nothing")

      )



      expect(data_from(context)).to include(log_hash("project" => "p", "root_namespace" => "n", "user" => "u"))

      expect(data_from(context).keys).not_to include(described_class.log_key("something_else"))

      expect(data_from(context)).to include(log_hash("project" => "p", "root_namespace" => "n", "user" => "u", "something_else" => "nothing"))

    end



    it "always assigns a correlation id" do

spec/labkit/middleware/sidekiq/context/server_spec.rb

+15 −3
Original line number Diff line number Diff line
@@ -25,11 +25,23 @@ describe Labkit::Middleware::Sidekiq::Context::Server do 
    end



    it "sets the application context from job params and clears after running" do

      expected_metadata = { "meta.project" => "jane.doe/bookstore", "meta.user" => "jane.doe" }

      job = expected_metadata.merge("class" => "TestWorker", "queue" => "default", "args" => ["do it"])

      expected_metadata = {

        "correlation_id" => "123",

        "meta.project" => "jane.doe/bookstore",

        "meta.user" => "jane.doe",

        "meta.random" => "key",

        :"meta.symbol" => "included",

      }

      job = expected_metadata.merge(

        "class" => "TestWorker",

        "queue" => "default",

        "args" => ["do it"],

        "something_else" => "not prefixed with meta",

        symbol: "not included", # rubocop: disable Style/HashSyntax explicitly testing mixed hash

      )



      expect(Labkit::Context).to receive(:with_context)

                                   .with(hash_including(job)).ordered.and_call_original

                                   .with(hash_including(expected_metadata.stringify_keys)).ordered.and_call_original

      expect(fake_job).to receive(:perform).with("do it").ordered



      Sidekiq::Client.push(job)