optionally include IP info at start of search/find linked data requests #302
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Marks the start of each linked data search/find request. Unless `Qa.config.suppress_ip_data_from_log`, this marker will include IP address, city, state, and country as reported by the `geocoder` gem. NOTE: This also removes inclusion of the request_id directly in the logging messages. To include request_id, it is recommended that you set up the logger to do this automatically in `config/environments/development.rb` (or relevant env file) using… ``` config.log_tags = [:request_id] ```
elrayle
commented
Dec 13, 2019
| @@ -16,7 +17,8 @@ class RequestHeaderService | |||
| def initialize(request:, params:) | |||
| @request = request | |||
| @params = params | |||
| @request_id = assign_request_id | |||
| @request_id = request.request_id | |||
There was a problem hiding this comment.
no need to assign a new one since each request comes with an id
elrayle
commented
Dec 13, 2019
| @id = id | ||
| url = authority_service.build_url(action_config: term_config, action: :term, action_request: normalize_id, request_header: request_header) | ||
| Rails.logger.info "#{request_id} - QA Linked Data term url: #{url}" | ||
| Rails.logger.info "QA Linked Data term url: #{url}" |
There was a problem hiding this comment.
There are several places where #{request_id} is removed. The same effect can be achieved with standard logging features. See PR notes for more info.
tworrall
approved these changes
Dec 16, 2019
elrayle
commented
Dec 16, 2019
| @@ -2,6 +2,7 @@ | |||
| require 'json' | |||
|
|
|||
| describe Qa::LinkedDataTermsController, type: :controller do | |||
| let(:graph_load_failure) { /^\*\*\*\*\*\*\*\* RDF\:\:Graph\#load failure/ } | |||
There was a problem hiding this comment.
Pattern for catching new graph load failure logging. Exact failure is will be driven by the exception thrown and does not require validation here. The required validation is that the logging occurred. This pattern allows for that validation.
|
Is it a concern that geocode-lookup of every IP address for log line may take non-trivial amounts of time, and add to response times when such logging is turned on? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Marks the start of each linked data search/find request. Unless
Qa.config.suppress_ip_data_from_log, this marker will include IP address, city, state, and country as reported by thegeocodergem.NOTE: This also removes inclusion of the request_id directly in the logging messages. To include request_id, it is recommended that you set up the logger to do this automatically in
config/environments/development.rb(or relevant env file) using…