Patch logger to match Superlogger format, insert instrumentation logs #68

jeremyyap · 2017-01-13T14:46:08Z

Patch ActiveSupport::Logger::SimpleFormatter to match Superlogger format
Patch Flexirest gem to remove ANSI escape sequences for text color
Insert log lines at the following positions:
- Evaluation job retrieved
- Evaluation package retrieved
- Evaluation job completed

houndci-bot · 2017-01-13T14:46:15Z

lib/simple_formatter_patch.rb

+    caller_location = get_caller_location
+    args = format_args(msg)
+
+    "#{formatted_time} | CMEvaluator | CMEvaluator | #{formatted_severity} | #{caller_location} | #{args}\n"


Line is too long. [108/100]

houndci-bot · 2017-01-13T14:46:16Z

lib/simple_formatter_patch.rb

+class ActiveSupport::Logger::SimpleFormatter
+  def call(severity, time, progname, msg)
+    formatted_severity = severity[0]
+    formatted_time = time.strftime("%Y-%m-%d %H:%M:%S.") << time.usec.to_s[0..2].rjust(3)


Prefer single-quoted strings when you don't need string interpolation or special symbols.

houndci-bot · 2017-01-13T14:46:16Z

lib/simple_formatter_patch.rb

+ActiveSupport::LogSubscriber.colorize_logging = false
+
+class ActiveSupport::Logger::SimpleFormatter
+  def call(severity, time, progname, msg)


Unused method argument - progname. If it's necessary, use _ or _progname as an argument name to indicate that it won't be used.

houndci-bot · 2017-01-13T14:46:16Z

lib/flexirest_patch.rb

+        Flexirest::Logger.debug "#{Flexirest.name} #{key} - Writing to cache"
+        cached_response = CachedResponse.new(status:response.status, result:result, response_headers: headers)
+        cached_response.etag = "#{headers[:etag]}" if headers[:etag]
+        cached_response.expires = Time.parse(headers[:expires]) rescue nil if headers[:expires]


Avoid using rescue in its modifier form.

houndci-bot · 2017-01-13T14:46:17Z

lib/flexirest_patch.rb

+        key = "#{request.class_name}:#{request.original_url}"
+        Flexirest::Logger.debug "#{Flexirest.name} #{key} - Writing to cache"
+        cached_response = CachedResponse.new(status:response.status, result:result, response_headers: headers)
+        cached_response.etag = "#{headers[:etag]}" if headers[:etag]


Prefer to_s over string interpolation.

houndci-bot · 2017-01-13T14:46:17Z

lib/flexirest_patch.rb

+      if cache_store && (headers[:etag] || headers[:expires])
+        key = "#{request.class_name}:#{request.original_url}"
+        Flexirest::Logger.debug "#{Flexirest.name} #{key} - Writing to cache"
+        cached_response = CachedResponse.new(status:response.status, result:result, response_headers: headers)


Space missing after colon.
Line is too long. [110/100]

houndci-bot · 2017-01-13T14:46:17Z

lib/flexirest_patch.rb

+      return unless !result.respond_to?(:_status) || [200, 304].include?(result._status)
+      headers = response.response_headers
+
+      headers.keys.select{|h| h.is_a? String}.each do |key|


Space missing to the left of {.
Space between { and | missing.
Space missing inside }.

houndci-bot · 2017-01-13T14:46:17Z

lib/flexirest_patch.rb

+        key = "#{request.class_name}:#{request.original_url}"
+        Flexirest::Logger.debug "#{Flexirest.name} #{key} - Trying to read from cache"
+        value = cache_store.read(key)
+        value = Marshal.load(value) rescue value


Useless assignment to variable - value.
Avoid using rescue in its modifier form.

houndci-bot · 2017-01-13T14:46:18Z

lib/flexirest_patch.rb

+
+    def disable_automatic_date_parsing=(value)
+      Flexirest::Logger.info "#{name} Request Body Type set to be #{value}"
+      @@disable_automatic_date_parsing = value


Replace class var @@disable_automatic_date_parsing with a class instance var.

houndci-bot · 2017-01-13T14:46:18Z

lib/flexirest_patch.rb

+
+    def request_body_type=(value)
+      Flexirest::Logger.info "#{name} Request Body Type set to be #{value}"
+      @@request_body_type = value


Replace class var @@request_body_type with a class instance var.

houndci-bot · 2017-01-13T14:46:18Z

lib/flexirest_patch.rb

+    def password=(value)
+      Flexirest::Logger.info "#{name} Password set..."
+      value = CGI::escape(value) if value.present? && !value.include?("%")
+      @@password = value


Replace class var @@password with a class instance var.

houndci-bot · 2017-01-13T14:46:18Z

lib/flexirest_patch.rb

+
+    def password=(value)
+      Flexirest::Logger.info "#{name} Password set..."
+      value = CGI::escape(value) if value.present? && !value.include?("%")


Do not use :: for method calls.
Prefer single-quoted strings when you don't need string interpolation or special symbols.

houndci-bot · 2017-01-13T14:46:18Z

lib/flexirest_patch.rb

+    def username=(value)
+      Flexirest::Logger.info "#{name} Username set to be #{value}"
+      value = CGI::escape(value) if value.present? && !value.include?("%")
+      @@username = value


Replace class var @@username with a class instance var.

houndci-bot · 2017-01-13T14:46:19Z

lib/flexirest_patch.rb

+
+    def username=(value)
+      Flexirest::Logger.info "#{name} Username set to be #{value}"
+      value = CGI::escape(value) if value.present? && !value.include?("%")


Do not use :: for method calls.
Prefer single-quoted strings when you don't need string interpolation or special symbols.

houndci-bot · 2017-01-13T14:46:19Z

lib/flexirest_patch.rb

+    def base_url=(value)
+      Flexirest::Logger.info "#{name} Base URL set to be #{value}"
+      value = value.gsub(/\/+$/, '')
+      @@base_url = value


Replace class var @@base_url with a class instance var.

houndci-bot · 2017-01-13T14:46:19Z

lib/flexirest_patch.rb

+        request_options[:api_auth] = {
+          :api_auth_access_id => api_auth_access_id,
+          :api_auth_secret_key => api_auth_secret_key,
+          :api_auth_options => api_auth_options


Use the new Ruby 1.9 hash syntax.

houndci-bot · 2017-01-15T14:20:09Z

lib/coursemology/evaluator/models/programming_evaluation.rb

    @package ||= begin
      body = self.class._plain_request('courses/assessment/programming_evaluations/:id/package',
                                       :get, id: id)
+      Coursemology::Evaluator.logger.info({event: 'Evaluation package received', evaluation_id: id})


Redundant curly braces around a hash parameter.
Space inside { missing.
Space inside } missing.

houndci-bot · 2017-01-15T14:20:09Z

lib/coursemology/evaluator/models/programming_evaluation.rb

    @package ||= begin
      body = self.class._plain_request('courses/assessment/programming_evaluations/:id/package',
                                       :get, id: id)
+      Coursemology::Evaluator.logger.info({event: 'Evaluation package received', evaluation_id: id})


Redundant curly braces around a hash parameter.
Space inside { missing.
Space inside } missing.

houndci-bot · 2017-01-15T14:20:09Z

lib/coursemology/evaluator/client.rb

      evaluation.evaluate
    end

+    Coursemology::Evaluator.logger.info({event: 'Evaluation job completed', evaluation_id: evaluation.id})


Redundant curly braces around a hash parameter.
Space inside { missing.
Line is too long. [106/100]
Space inside } missing.

houndci-bot · 2017-01-15T14:20:09Z

lib/coursemology/evaluator/client.rb

      evaluation.evaluate
    end

+    Coursemology::Evaluator.logger.info({event: 'Evaluation job completed', evaluation_id: evaluation.id})


Redundant curly braces around a hash parameter.
Space inside { missing.
Line is too long. [106/100]
Space inside } missing.

houndci-bot · 2017-01-15T14:20:09Z

lib/coursemology/evaluator/client.rb

  # @param [Coursemology::Evaluator::Models::ProgrammingEvaluation] evaluation The evaluation
  #   retrieved from the server.
  def on_evaluation(evaluation)
+    Coursemology::Evaluator.logger.info({event: 'Evaluation job retrieved', evaluation_id: evaluation.id})


Redundant curly braces around a hash parameter.
Space inside { missing.
Line is too long. [106/100]
Space inside } missing.

houndci-bot · 2017-01-15T14:20:09Z

lib/coursemology/evaluator/client.rb

  # @param [Coursemology::Evaluator::Models::ProgrammingEvaluation] evaluation The evaluation
  #   retrieved from the server.
  def on_evaluation(evaluation)
+    Coursemology::Evaluator.logger.info({event: 'Evaluation job retrieved', evaluation_id: evaluation.id})


Redundant curly braces around a hash parameter.
Space inside { missing.
Line is too long. [106/100]
Space inside } missing.

weiqingtoh · 2017-01-16T02:03:28Z

lib/simple_formatter_patch.rb

+                 "#{key}=#{value}"
+               end.join(' | ')
+             else
+               args.to_s.gsub(/\033\[1;4;32m(.*)\033\[0m/, '\1')


Do we need to add that this is msg=foo?

This follows the behaviour of Superlogger:

If it receives a hash it will print out as msg=foo
Otherwise, it will just print out the msg converted to string.

fonglh · 2017-01-16T23:23:48Z

Is it possible to make use of ActiveSupport::Notifications.instrument? Saw some of these calls in the code already.

houndci-bot reviewed Jan 13, 2017

View reviewed changes

houndci-bot reviewed Jan 15, 2017

View reviewed changes

Patch logger to match Superlogger format, insert instrumentation logs

ca0b5c4

weiqingtoh reviewed Jan 16, 2017

View reviewed changes

jeremyyap closed this Jan 17, 2017

Patch logger to match Superlogger format, insert instrumentation logs #68

Patch logger to match Superlogger format, insert instrumentation logs #68

Uh oh!

Conversation

jeremyyap commented Jan 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fonglh commented Jan 16, 2017

Uh oh!

Uh oh!