feat(job_start_log): flag to enable logging of job start

[mdibaiee]

fixes #4

[iMacTia]

@mdibaiee thanks for opening this PR 🎉 !
Changes look good but it would be great to see some new tests that shows the new feature works as expected 😃

[mdibaiee]

@iMacTia Thanks. I agree tests would be great, I just added two.

I noticed there seems to be seemingly duplicate code over here and here. I have changed the first one only now, and my manual tests were fine (Sidekiq was indeed logging start of jobs). I'm wondering how these two pieces of code related to each other.

[iMacTia]

@mdibaiee thanks for adding the tests and for pointing out the code duplication.
I checked the history and I noticed it was introduced with this commit
I'm quite sure one of the two file is a leftover at this point, but will require additional checks before removing it, so don't worry about it for now 👍, I'll open an issue for it.

I'll review this PR again now

[iMacTia]

Looks good and is code-covered now 👍
@mdibaiee I'd also add a test to ensure that the the starting log is NOT logged when the option is disabled, but to be honest is not a showstopper, so make it a "nice to have" 😄

@silviusimeria would you like to review as well?

[mdibaiee]

Thanks @iMacTia . I just addd a test case for "not logging" a job_start log by default. 👍

[silviusimeria]

Hi guys,

The logging server middleware was removed in Sidekiq 5.0.0, see: https://github.com/mperham/sidekiq/blob/master/Changes.md

If I remember correctly, when I added support for sidekiq 5, the common logging logic was moved inSidekiq::Logging::Shared, but some parts remained duplicated because I wanted this gem to support older sidekiq versions through the middleware approach.

If you want to port the job start log to older sidekiq versions you might need to change the middleware part too.

[mdibaiee]

@iMacTia so, what's new here? do I need to make additional changes?

[iMacTia]

@mdibaiee: @silviusimeria point was that the changes you did to logstash_job_logger.rb needs to be done to server/logstash_logging.rb as well if you want to support Sidekiq < 5.0.
If this is something you're not interested in, I can always merge your PR and we can extend the feature to older versions of Sidekiq on a separate PR. Just let me know :)

[mdibaiee]

@iMacTia I see, we use Sidekiq-5.x.x, so I think we're good with the current functionality. Testing the change on older Sidekiq versions would also be of additional effort and I would rather skip that. 😁

Looking forward to using this in our production system 🚀

[iMacTia]

@mdibaiee sure no problem! Thanks for contributing!
I’ll merge this so you can use master branch in the meantime, but I’d rather get the older versions and code duplication fixed before releasing a proper version.
Hopefully I’ll be able to look at it sometime this or next week :)
Cc @silviusimeria