Add Dj plug in to support tagging dj logs #4
Conversation
wshihadeh
force-pushed
the
enable_setting_request_id
branch
from
1ed1528
to
c3d118a
Compare
spec/delayed_job_spec.rb
Outdated
| ) | ||
| expect(dj.object.class).to eq(User) | ||
| expect(dj.method_name).to eq(:send_email) | ||
| expect(dj.instance_variable_defined?(:@store)).to eq(true) |
There was a problem hiding this comment.
You don't need that assertion IMHO.
lib/request_headers_middleware.rb
Outdated
| RequestStore[:headers] = store | ||
| end | ||
|
|
||
| def tag_logger(logger) |
There was a problem hiding this comment.
This adds an implicit dependency to ActiveSupport::TaggedLogger AFAIK. Right? Is it necessary? Should it not be handled outside?
I never thought that all store items get into the tagged logging. My idea was to use the request_headers_middleware as a store where you can store different data.
There was a problem hiding this comment.
1- Yes this add the dependency on ActiveSupport::TaggedLogger
2- We need it to be able to create the tags on the logger, otherwise logs will not show the x-request id.
3- I put it here, to be able to reuse it for other plugins (ie MQ). But we can move it also to the plugin file if that makes more sense. What do you think about it?
4- I am tagging all store items because I did not want to do it explicitly. for 'X-request-id' like this
logger.push_tags(store[:'X-Request-Id']) unless store[:'X-Request-Id'].nil?
does it make since moving this to the delayed job plugin and make the tagging only for 'X-Request-Id'
BTW, this PR will also restore the lost commit '3d8f2ec'
wshihadeh
changed the base branch from
master
to
hotfix/failback_to_default_hash
wshihadeh
changed the base branch from
hotfix/failback_to_default_hash
to
master
wshihadeh
force-pushed
the
enable_setting_request_id
branch
from
1030dab
to
21296c0
Compare
|
@MarcGrimme I moved the tag function to the plugin. if you still think that this is not the place for it, then the only option is to create new gem for it |
.travis.yml
Outdated
| - "2.3.3" | ||
|
|
||
|
|
||
| before_install: gem install bundler -v 1.14.6 |
There was a problem hiding this comment.
Why do you fix the version?
Gemfile
Outdated
| source 'https://rubygems.org' | ||
|
|
||
| # Specify your gem's dependencies in request_store.gemspec | ||
| gemspec | ||
|
|
||
| group :test, :development do | ||
| gem 'activesupport' |
There was a problem hiding this comment.
I'd move those in the gemspec.
| @@ -0,0 +1,58 @@ | |||
| # frozen_string_literal: true | |||
There was a problem hiding this comment.
I'd really move this part into its own gem like I did for logstasher-plugin-delayed. Two reasons:
- It's an unnecessary dep
- It adds a dep to TaggedLogging
- It adds a dep to X-Request-ID
that has no connection to request_headers_middleware
|
|
||
| def self.tag_logger(logger) | ||
| uuid = RequestHeadersMiddleware.store[:"X-Request-Id"] | ||
| logger.push_tags(uuid) unless uuid.nil? |
There was a problem hiding this comment.
This is dangerous because it adds the dependency to TaggedLogging that is only there for Rails or better ActiveSupport. Plus what happens if we do JSon Logging. I think also this needs to be moved into some pluggable env. I tried to solve it in my message_queue PR.
spec/delayed_request_plugin_spec.rb
Outdated
|
|
||
| tags = logger.pop_tags(store.size) | ||
| expect(tags.count).to eq(1) | ||
| expect(tags.first).to eq(store[:'X-Request-Id']) |
There was a problem hiding this comment.
Here I'd also test for the right x-request-id.
| @@ -17,6 +20,12 @@ | |||
| expect(RequestHeadersMiddleware.store).to eq({}) | |||
| end | |||
|
|
|||
| it 'assign a value for store and tag logger' do | |||
There was a problem hiding this comment.
I don't know if this test is really required.
wshihadeh
force-pushed
the
enable_setting_request_id
branch
from
5ed2f99
to
013d4ae
Compare
|
@MarcGrimme I moved the plugin to a new gem fidor/request_headers_logger#1 |
https://jira.fidor.de/browse/DEVOPS-790