Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Sidekiq logging is missing context information #51

Closed
prikha opened this issue Feb 12, 2017 · 17 comments
Closed

Sidekiq logging is missing context information #51

prikha opened this issue Feb 12, 2017 · 17 comments

Comments

@prikha
Copy link

prikha commented Feb 12, 2017

Following the readme I`ve added semantic logger to Sidekiq.

Sidekiq::Logging.logger = SemanticLogger[Sidekiq] if defined?(Sidekiq)

And it occurred that i`ve lost the most valuable data - worker class name.

The log before:

2017-02-12T19:18:43.908Z 22548 TID-osbolpr4g SomeWorker JID- INFO: start 2017-02-12T19:18:43.912Z 22548 TID-osbolpr4g SomeWorker JID- INFO: done: 0.003 sec

The log after:

{"name":"Sidekiq","pid":19857,"thread":"47328718486600","level":"info","level_index":2,"host":"web01","application":"Semantic Logger","message":"start","timestamp":"2017-02-12T19:32:00.599277Z"} {"name":"Sidekiq","pid":19857,"thread":"47328718486600","level":"info","level_index":2,"host":"web01","application":"Semantic Logger","message":"done: 0.002 sec","timestamp":"2017-02-12T19:32:00.601137Z"}

Is there a known way to capture the name of the calling worker class. Or maybe we should provider some facilities for such a broad use-case?

@reidmorrison
Copy link
Owner

reidmorrison commented Feb 15, 2017

It looks like Sidekiq tries to add context information in its own way and does not use builtin features such as tags to add context to log entries.

Recommend patching the Sidekiq logging class using an initializer and make it use tags instead of a custom tags mechanism.

Then you can also get rid of the string output, and log the individual elements as json attributes that do not have to be parsed by the centralized log system.

Have you looked at RocketJob ? It has complete support for Semantic Logger built-in from the ground up. It logs each classes name along with the duration of each task, etc..

@reidmorrison reidmorrison changed the title Sidekiq logging is unusable Sidekiq logging is missing context information Feb 15, 2017
@prikha
Copy link
Author

prikha commented Feb 15, 2017

Thanks for support!

Got it, RocketJob looks very promising. To be fair you should include Sidekiq Pro feature in the comparision table.

Went with a sample like this

# config/initializers/sidekiq.rb
module Sidekiq
  module Middleware
    module Server
      class Logging
        SPACE = ' '.freeze

        def call(worker, item, queue)
          Sidekiq::Logging.with_context(log_context(worker, item)) do
            # Apply payload to semantic logger
            # * we are loosing formatter capabilities
            logger.with_payload(log_payload(worker, item)) do
              begin
                start = Time.now
                logger.info("start".freeze)
                yield
                logger.info("done: #{elapsed(start)} sec")
              rescue Exception
                logger.info("fail: #{elapsed(start)} sec")
                raise
              end
            end
          end
        end

        private

        # If we're using a wrapper class, like ActiveJob, use the "wrapped"
        # attribute to expose the underlying thing.
        def log_context(worker, item)
          klass = item['wrapped'.freeze] || worker.class.to_s
          "#{klass} JID-#{item['jid'.freeze]}#{" BID-#{item['bid'.freeze]}" if item['bid'.freeze]}"
        end

        # This additional method formats context as payload
        def log_payload(worker, item)
          {
            class_name: item['wrapped'.freeze] || worker.class.to_s,
            jid: item['jid'.freeze],
            tid: Thread.current.object_id.to_s(36),
            context: context
          }
        end

        def context
          c = Thread.current[:sidekiq_context]
          " #{c.join(SPACE)}" if c && c.any?
        end

        def elapsed(start)
          (Time.now - start).round(3)
        end

        def logger
          Sidekiq.logger
        end
      end
    end
  end
end

@prikha prikha closed this as completed Feb 15, 2017
@reidmorrison
Copy link
Owner

Thanks for the code extract, happy to hear it works now.

By supporting RocketJob Pro you will also be supporting the development efforts on Semantic Logger and Symmetric Encryption.

@gingerlime
Copy link
Contributor

@prikha thanks for the snippet. Is this something that can be contributed to Sidekiq? so it generically works with "plain" logs as well more sophisticated ones, like SemanticLogger or maybe others?

(are tags something common to all loggers? or something specific to SemanticLogger)?

@gingerlime
Copy link
Contributor

gingerlime commented Mar 10, 2017

p.s. I hope that any rivalry between Sidekiq and RocketJob can be put aside for the purpose of better logging, irrespective of which product / logger is used ;-)

@prikha
Copy link
Author

prikha commented Mar 14, 2017

@gingerlime Hi, I had a quick run over sidekiq logging code - it seems very specific to what they are doing. Generalizing it for better logging solutions(better than plain ruby logger) should be a right direction imho.

However tagged logging is being introduced by ActiveSupport which is required by rails. Sidekiq itself has nothing to do with rails.

For me it seems there are plenty of options like message formatting which is done right. It would be great to have an opportunity to define not a custom formatter, but a block of code which consumes all message events and actually calls logger(in all its variety).

Anyway - there is not a right place for that discussion. We may open an issue at relevant repo.

@gingerlime
Copy link
Contributor

Thanks @prikha ... I'm not sure I completely understand what's the best way forward here.

Regardless if Sidekiq isn't super-flexible with custom loggers, I still wonder why, if I just want a plain-text file log output, I would somehow lose some of the context info that Sidekiq outputs. And with any other logger, it just works out-of-the-box (right?)

@gingerlime
Copy link
Contributor

@prikha @reidmorrison sorry to bother you guys, but I'm looking at some examples, but can't quite wrap my head around it fully.

If I look at the JSON formatter, then the usage section says I should assign the formatter to the sidekiq logger. The formatter itself inherits from Sidekiq's Pretty formatter, which outputs everything as a string.

However, when I try to use the standard pretty formatter by assigning it, SemanticLogger either doesn't use it, or ignores it? I don't see all the info I'd expect in my logs

# config/initializers/sidekiq.rb
Sidekiq.logger.formatter = Sidekiq::Logging::Pretty.new

log output:

2017-04-05 11:51:54.650201 I [4391:47191573070460] Sidekiq -- start
2017-04-05 11:51:55.381212 I [4391:47191573070460] Sidekiq -- done: 0.731 sec

Without SemanticLogger

2017-04-05T09:16:01.343Z 18637 TID-1f5h70 ClassName JID-936bd16c703db2ffbc9b2b5a INFO: start
2017-04-05T09:16:05.392Z 18637 TID-1f5h70 ClassName JID-936bd16c703db2ffbc9b2b5a INFO: done: 4.049 sec

I can create a middleware like @prikha suggested, or using Rails tags similar to this?

But I just wonder where's the conflict, or why the full log record isn't logged correctly by default? when I'm using the logging-rails gem, and lograge, it "just works" with those. So why can't this happen with Semantic Logger?

... sorry if it's a silly question, but I'm genuinely confused about how this all fits together, or doesn't as the case might be ;-)

@reidmorrison
Copy link
Owner

Sidekiq creates its own custom logger that is not compatible with standard loggers. The only way around it is to "monkey-patch" sidekiq as per the example above to use semantic logger instead of using its own custom logger.

If Sidekiq followed either Semantic Logger or the built-in Rails logger approach of using logger.tagged for tagged logging then this would not be an issue.

@gingerlime
Copy link
Contributor

gingerlime commented Apr 5, 2017

@reidmorrison regarding tagged, as far as I understand from @prikha, then this is a rails feature, and therefore I can see why it wouldn't use it by default.

Correct me if I'm wrong, but the sidekiq logger is compatible with the Ruby Logger (at least according to the wiki page), and it does work with other logging libraries, doesn't it?

Is there a change to the sidekiq logger that would make it compatible with standard loggers? then maybe it's worth submitting a PR to sidekiq to bring this compatibility?

I'm really just trying to understand why or what's not compatible and hopefully find a way for SemanticLogger and Sidekiq logger to work together "out of the box" if possible.

@gingerlime
Copy link
Contributor

This is what I'm using now (similar to the snippet above, but slightly different), and it seems to work with rails using logger.tagged, but I still hope there's some reasonable change somewhere that would make it work out-of-the-box...

module Sidekiq
  module Middleware
    module Server
      class TaggedLogging
        SPACE = ' '.freeze

        def call(worker, item, _queue)
          logger.tagged(log_context(worker, item)) do
            begin
              start = Time.zone.now
              logger.info('start'.freeze)
              yield
              logger.info("done: #{elapsed(start)} sec")
            rescue Exception
              logger.info("fail: #{elapsed(start)} sec")
              raise
            end
          end
        end

        private

        # If we're using a wrapper class, like ActiveJob, use the "wrapped"
        # attribute to expose the underlying thing.
        def log_context(worker, item)
          klass = item['wrapped'.freeze] || worker.class.to_s
          "#{klass} JID-#{item['jid'.freeze]}#{" BID-#{item['bid'.freeze]}" if item['bid'.freeze]}"
        end

        def context
          c = Thread.current[:sidekiq_context]
          " #{c.join(SPACE)}" if c && c.any?
        end

        def elapsed(start)
          (Time.zone.now - start).round(3)
        end

        def logger
          Sidekiq.logger
        end
      end
    end
  end
end

Sidekiq.configure_server do |config|
  config.server_middleware do |chain|
    chain.add Sidekiq::Middleware::Server::TaggedLogging
  end
end

@Silex
Copy link
Contributor

Silex commented Jul 20, 2017

Ok, I confirm the Sidekiq logger is weird. If you replace it by Logger.new('testfile.log'), this is what gets logged:

I, [2017-07-20T16:46:27.359965 #14]  INFO -- : Running in ruby 2.4.1p111 (2017-03-22 revision 58053) [x86_64-linux]
I, [2017-07-20T16:46:27.360022 #14]  INFO -- : See LICENSE and the LGPL-3.0 for licensing details.
I, [2017-07-20T16:46:27.360037 #14]  INFO -- : Upgrade to Sidekiq Pro for more features and support: http://sidekiq.org
I, [2017-07-20T16:46:27.361560 #14]  INFO -- : Starting processing, hit Ctrl-C to stop
I, [2017-07-20T16:46:27.307104 #15]  INFO -- : start
I, [2017-07-20T16:46:27.732218 #15]  INFO -- : done: 0.425 sec

So, all information is lost with regular Logger as well. Recently there was a way to customize the job logger class, investiguating atm.

EDIT: okay basically with Sidekiq you are supposed to use a custom format in order to get things logged:

"#{time.utc.iso8601} #{Process.pid} TID-#{Thread.current.object_id.to_s(36)}#{context} #{severity}: #{message}\n"

@lacco
Copy link

lacco commented Jan 26, 2018

Based on @gingerlime 's idea, I was able to customize the logging via overwriting Sidekiq::JobLogger:

class CustomSidekiqJobLogger < Sidekiq::JobLogger
  def call(item, queue, &block)
    logger.tagged(*context_tags(item)) do
      super(item, queue, &block)
    end
  end

  private

  def context_tags(item)
    [
      item['jid'],
      item['class']
    ]
  end

  def logger
    Rails.logger
  end
end

Sidekiq.configure_server do |config|
  config.options[:job_logger] = CustomSidekiqJobLogger
end

@prdanelli
Copy link

For anyone else finding this thread, none of the solutions offered here or #80 worked for me.

Semantic logger was preventing Sidekiq from outputting anything at all.

I added Sidekiq.logger = Logger.new(STDOUT) in initializers/sidekiq.rb to force Sidekiq to use the default logger to fix the issue.

@gingerlime
Copy link
Contributor

I'm still playing around with this, but with Sidekiq 6.0.1 @lacco example can be boiled down to

class TaggedJobLogger < Sidekiq::JobLogger
  def call(item, queue)
    @logger.tagged(Sidekiq::Context.current) do
      super(item, queue)
    end
  end
end

...

Sidekiq.configure_server do |config|
  ...

  config.options[:job_logger] = TaggedJobLogger

  ...
end

@bobvanderlinden is this more or less what you had in mind ?

@bobvanderlinden
Copy link

Yes, indeed. I was thinking it might be nice to use such a tagged logger for Sidekiq by default.

@ixti
Copy link

ixti commented Jul 1, 2022

Notice, that Sidekiq.logger = SemanticLogger[Sidekiq] extends given logger with Sidekiq::LoggingUtils. That module provides #log_at (similar to SemanticLogger#silence, and that is used by JobLogger if job payload includes log_level) and #add method that relies on @logdev instance variable...

Repository owner locked and limited conversation to collaborators Jul 5, 2022
@reidmorrison reidmorrison converted this issue into discussion #221 Jul 5, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests

8 participants