Optimize thread usage and handle http exceptions. #7

vilyapilya · 2019-08-22T22:48:12Z

Most of the changes are in the client file. I removed the scheduling library bc it did not do the timing.
I also changed the names of some variable, which we will probably need to evert to the old ones if if we have a lot customer using this library.

smusali

Let's try to make it consistent with Node.js Code Library regarding the configurable options...

lib/logdna/resources.rb

dchai76

This is first pass, I haven't looked at the actual buffer logic yet. Main comments are:

Indentation is inconsistent - can you fix that?
It looks like there's at least one stray debug puts, can you make sure there's nothing else extraneous?
I'm a little confused by the stuff you pulled out, mostly exception handling and cleanup logic. Can you explain why you did?

lib/logdna.rb

dchai76 · 2019-09-04T19:46:30Z

lib/logdna/client.rb

-      @actual_flush_interval = opts[:flushtime] ||= Resources::FLUSH_INTERVAL
-
-      @@request = request
+      @flush_limit = opts[:flush_size] ||= Resources::FLUSH_BYTE_LIMIT


I'm pretty sure we're misusing ||= here. That operator sets the variable if it's falsy. So what we're doing here is setting opts[:flush_size] to Resources::FLUSH_BYTE_LIMIT if it's not already set, then setting @flush_limit to that value. I don't see the opts being used again, I think we just want to set @flush_limit, so @flush_limit = opts[:flush_size] || Resources::FLUSH_BYTE_LIMIT. Same below in line 22 and anywhere else we're using ||=.

no it does assign the value to @flush_limit.

#a = 9 test = a ||= 1 puts test -> 1 a = 9 test = a ||= 1 puts test -> 9

dchai76 · 2019-09-04T19:47:37Z

lib/logdna/client.rb

-        else
-          # no messages means we can kill the task
-          task.kill
+        timer_task = Concurrent::TimerTask.new(execution_interval: @flush_interval, timeout_interval: Resources::TIMER_OUT) do |task|


Indent is off here - it's 2 spaces everywhere else, 4 spaces here.

dchai76 · 2019-09-04T19:47:55Z

lib/logdna/client.rb

-          # no messages means we can kill the task
-          task.kill
+        timer_task = Concurrent::TimerTask.new(execution_interval: @flush_interval, timeout_interval: Resources::TIMER_OUT) do |task|
+            puts 'executing'


This looks like a stray debug message?

dchai76 · 2019-09-04T19:48:55Z

lib/logdna/client.rb

-      write_to_buffer(next_object[:msg], next_object[:opts])
-    end
+      if @lock.try_lock
+          if !@side_messages.empty?


Same note about indent, it's inconsistent where you're using 2 or 4

dchai76 · 2019-09-04T19:49:50Z

lib/logdna/client.rb

+        begin
+          @lock.unlock
+        rescue
+          puts 'Nothing was locked'


This seems not too useful

ok, I'll silent it

dchai76

Not all of previous comments were addressed, and adding a few more.

dchai76 · 2019-09-05T21:29:27Z

lib/logdna/client.rb

-    def message_hash(msg, opts={})
-      obj = {
+    def process_message(msg, opts={})
+      processedMessage = {


Use underscores, please.

dchai76 · 2019-09-05T21:32:04Z

lib/logdna/client.rb

-          @task.shutdown
-          @task.kill
+          if(@response.is_a?(Net::HTTPForbidden))
+            p "Please provide a valid ingestion key"


Please be consistent and use puts everywhere

dchai76 · 2019-09-05T21:32:24Z

lib/logdna/client.rb

-        unless @buffer_over_limit || @messages.any? || @task.nil?
-          @task.shutdown
-          @task.kill
+          if(@response.is_a?(Net::HTTPForbidden))


(nit) Space after the if. This goes for everywhere in this block below.

lib/logdna.rb

dchai76 · 2019-09-06T19:26:54Z

lib/logdna.rb

-        return
-      end
+      request = Net::HTTP::Post.new(uri.request_uri, 'Content-Type' => 'application/json')
+      request.basic_auth('username', key)


Does this call possibly raise an exception? What should we do?

no, it does not.

lib/logdna.rb

dchai76 · 2019-09-06T19:29:06Z

lib/logdna.rb

-      message = yield if msg.nil? && block_given?
-      @response = @@client.buffer(message, default_opts.merge(opts).merge({
+    def log(message, opts={})
+      if (message.length == 0)


Something like if (message.empty?) is more Ruby-ish.

dchai76 · 2019-09-06T19:31:21Z

lib/logdna.rb

+      end
+      message = message.to_s unless message.instance_of? String
+      message = message.encode("UTF-8")
+      @@client.write_to_buffer(message, default_opts.merge(opts).merge({


There is no authorization.

I'm not following - previously if request.basic_auth failed, @@client could be nil. We're not checking for that failure anymore so I'm not sure what happens now.

lib/logdna.rb

dchai76 · 2019-09-06T19:36:03Z

lib/logdna/client.rb

-
-    def encode_message(msg)
-      msg = msg.to_s unless msg.instance_of? String
+      @flush_limit = opts[:flush_size] ? opts[:flush_size] : Resources::FLUSH_BYTE_LIMIT


Why did you change this format? @flush_limit = opts[:flush_size] || Resources::FLUSH_BYTE_LIMIT is more typical.

I thought this what you requested the last time.

I was requesting we use || instead of ||=.

lib/logdna/client.rb

.ruby-version

lib/logdna.rb

vilyapilya · 2019-09-27T00:33:49Z

lib/logdna.rb

-        return
-      end
+      request = Net::HTTP::Post.new(uri.request_uri, 'Content-Type' => 'application/json')
+      request.basic_auth('username', key)


no, it does not.

vilyapilya · 2019-09-27T00:41:02Z

lib/logdna.rb

+      end
+      message = message.to_s unless message.instance_of? String
+      message = message.encode("UTF-8")
+      @@client.write_to_buffer(message, default_opts.merge(opts).merge({


This is the code library that writes a client that connects to our servers. Our servers use the ingestion keys for authenticating. (if a user has it, then he can send data to the server). Basic authentication does not make any sense here.

vilyapilya · 2019-09-27T00:43:07Z

lib/logdna/client.rb

-
-    def encode_message(msg)
-      msg = msg.to_s unless msg.instance_of? String
+      @flush_limit = opts[:flush_size] ? opts[:flush_size] : Resources::FLUSH_BYTE_LIMIT


I thought this what you requested the last time.

lib/logdna.rb

dchai76 · 2019-09-27T22:06:32Z

lib/logdna/client.rb

-    def encode_message(msg)
-      msg = msg.to_s unless msg.instance_of? String
+      @flush_limit = opts[:flush_size] ? opts[:flush_size] : Resources::FLUSH_BYTE_LIMIT
+      @flush_interval = opts[:flush_interval] ? opts[:flush_interval] : Resources::FLUSH_INTERVAL


Same thing here, I think you mistook my request. Use || instead of ||=. That's the semantic intention.

dchai76 · 2019-09-27T22:07:05Z

lib/logdna/client.rb

-      end
-      msg
+      @request = request
+      @retry_timeout = opts[:retry_timeout] ? opts[:retry_timeout] : Resources::RETRY_TIMEOUT


dchai76 · 2019-09-27T22:14:05Z

lib/logdna/client.rb

+      flush if @buffer.any? || @side_messages.any?
      puts "Exiting LogDNA logger: Logging remaining messages"
-      return
+      nil


Just curious why you changed this, I feel like I see empty return more than an ending nil.

because of linting ) but I can change its settings.

dchai76 · 2019-09-27T22:15:05Z

lib/logdna/client.rb

+    end
+
+    def exitout
+      flush if @buffer.any? || @side_messages.any?


This test is already a part of flush so redundant; let's remove it and just let flush do it.

dchai76 · 2019-09-27T22:18:43Z

lib/logdna/client.rb

    def write_to_buffer(msg, opts)
-      return if msg.nil?
-      msg = encode_message(msg)
+      if @lock.try_lock


So the way the lock logic has changed makes me a bit nervous. Before we just wrapped in a synchronize block so we know we released in the right place. Now there's odd coordination going on, and I'm not sure it's correct, like you're releasing the lock before we flush (line 62) - is that right? If you're concerned about blocking what about just checking for if @lock.locked? and adding to @side_messages in that case, otherwise doing this logic in a synchronize block?

dchai76 · 2019-09-27T22:19:54Z

lib/logdna/client.rb

+        if @response.is_a?(Net::HTTPForbidden)
+          puts "Please provide a valid ingestion key"
+        elsif !@response.is_a?(Net::HTTPSuccess)
+          puts "The response is not successful "


No retry in this case?

no, there is no point in retrying if the ingestion is not valid or not provided

But how do you know that that's why we didn't get a success response? What if there's some other error?

I know it does not look like an ideal handling. But testing how this mechanism works in Ruby, I found this to be the best approach. Because Ruby does not really work well with general exception handling. I tested it with writing just rescue, and switching off the connection and providing a bad ingestion key. The rescue part did not catch it. But it does catch if the specific exception names are defined. I can probably call handle_excpetion lambda in the second (!@response.is_a?(Net::HTTPSuccess)) condition.

This isn't an exception though, it's a non-success response. I'd consider a retry in this case - there's no harm.

lib/logdna/client.rb

dchai76 · 2019-09-27T22:23:37Z

lib/logdna/client.rb

-      @messages.push(message_hash(msg, opts))
-      buffer_size
-    end
+        @lock.unlock if @lock.locked?


OK, having read through this all, I now see why you did the unlock here and to me that's an indication that the lock logic is not right - it really should be locked/unlocked in roughly the same block - there's a somewhat crazy coordination game going on right now that makes the logic unclear, and I think not quite right.

dchai76

Just a comment on the yield block - let me review the lock logic tomorrow

dchai76 · 2019-10-10T01:02:21Z

lib/logdna.rb

-        self.log(msg, opts.merge({
-          level: lvl,
-        }), &block)
+      define_method name do |msg = nil, opts = {}|


So this is the part I wanted you to understand. What this is (or was) doing is defining methods for each log level error', warn, debug, etc. And in doing so, it's mimicking the methods that the Ruby Logger has: https://ruby-doc.org/stdlib-2.4.0/libdoc/logger/rdoc/Logger.html#method-i-info. If you look at all of those methods (I linked to info, but it's there for all of them), you'll see that they all take a &blockthatEvaluates to the message to log.. You're removing the functionality by removing the block handling in log`. If you want to do that we can, but note that it does not deviate from what Ruby's Logger looks like.

The original code does not replicate the Ruby Logger behavior. I'll make a change so it will d replicate it

Actually, it makes sense why the original code does not fully replicate the Ruby Logger behavior. I have restored it.

dchai76

It looks like you didn't finish addressing my previous comments but I did another round of review.

dchai76 · 2019-10-16T21:04:07Z

lib/logdna.rb

+    def log(message = nil, opts = {})
+      if message.nil? && block_given?
+        message = yield
+      elsif message.nil?


Might want to make this just if message.nil? to detect if the block provided is wrong.

dchai76 · 2019-10-16T21:12:38Z

lib/logdna.rb


      define_method "#{name}?" do
-        return Resources::LOG_LEVELS[self.level] == lvl if self.level.is_a? Numeric
+        return Resources::LOG_LEVELS[level] == lvl if level.is_a? Numeric


Did this work? I can't see where level is defined.

it did work but I'll change to self.level. I did not notice it

dchai76 · 2019-10-16T21:18:19Z

lib/logdna.rb

-      if defined? @@client and !@@client.nil?
-          @@client.exitout()
-      end
+      @client.exitout if defined? @client && !@client.nil?


Although optional, I prefer the style of always including parens on function calls for better readability.

dchai76 · 2019-10-16T21:20:50Z

lib/logdna.rb

-      if defined? @@client and !@@client.nil?
-          @@client.exitout()
-      end
+    def at_exit


Wait, at_exit is something you should be calling - it's the block that executes on program exit - not something you should be defining. https://apidock.com/ruby/Kernel/at_exit

dchai76 · 2019-10-16T21:25:49Z

lib/logdna/client.rb

-        @@request.body = real
-        @response = Net::HTTP.start(@uri.hostname, @uri.port, use_ssl: @uri.scheme == 'https') do |http|
-          http.request(@@request)
+      handle_excpetion = lambda do |message|


You said done... you didn't fix the spelling here and in every reference. Please do.

dchai76 · 2019-10-16T21:30:00Z

lib/logdna/client.rb

+    def flush
+      if @lock.try_lock
+        @flush_scheduled = false
+        return if @buffer.empty? && @side_messages.empty?


This seems wrong. You're returning without releasing the @lock. I think this could happen if 2 threads schedule a flush, one gets there first so empties the buffer/side_messages, then this thread gets here and never unlocks.

good point. Thanks

dchai76 · 2019-10-16T21:33:53Z

lib/logdna/client.rb

+        if @response.is_a?(Net::HTTPForbidden)
+          puts "Please provide a valid ingestion key"
+        elsif !@response.is_a?(Net::HTTPSuccess)
+          puts "The response is not successful "


This isn't an exception though, it's a non-success response. I'd consider a retry in this case - there's no harm.

dchai76 · 2019-10-16T21:34:38Z

logdna.gemspec

@@ -1,4 +1,5 @@
 # coding: utf-8
+


Prefer not having whitespace-only changes to files.

vilyapilya · 2019-10-17T20:48:44Z

I think "enhancement" is the wrong label here.

lib/logdna.rb

dchai76 · 2019-10-17T21:01:12Z

lib/logdna/client.rb

-        return
+    def send_request
+      @side_message_lock.synchronize do
+        @buffer.concat(@side_messages)


I think we should leave a comment here that this function must be called with a claimed @lock (and/or even explicitly check for that in the code). Otherwise when I read this I'm confused why we're modifying @buffer without looking at @lock.

dchai76 · 2019-10-17T21:07:16Z

lib/logdna/client.rb

+    def flush
+      if @lock.try_lock
+        @flush_scheduled = false
+        if @buffer.empty? && @side_messages.empty?


Alternatively, might want to reverse the logic so:

if @buffer.any? || @side_messages.any? send_request end @lock.unlock

That way there's just a single @lock.unlock call.

dchai76

OK! Let's make sure this gets good test coverage.

dchai76 · 2019-10-25T01:04:24Z

.gitignore

 *.gem
 .DS_Store
+/bin/
+package-lock.json


Ack no, this is back. Remove.

lvilya added 7 commits August 16, 2019 14:46

working on the scheduler

85cf794

savingBranch

5f4be72

the final test is insihed

4590d4e

add bin folder to gitignore

ef2c346

remive all bin folder changes"

e3b90ca

add tests

e430073

remove spec folder

5232804

smusali requested review from dchai76 and smusali September 4, 2019 15:09

smusali assigned vilyapilya Sep 4, 2019

smusali added the enhancement label Sep 4, 2019

smusali suggested changes Sep 4, 2019

View reviewed changes

lib/logdna/resources.rb Outdated Show resolved Hide resolved

remove unused method

1258ed4

dchai76 suggested changes Sep 4, 2019

View reviewed changes

add the flush flag check

bfef45f

vilyapilya changed the title ~~Optimize thread usage and handle http exceptions.~~ WIP:Optimize thread usage and handle http exceptions. Sep 4, 2019

lvilya added 3 commits September 4, 2019 14:38

not working here??

0a77127

not working chnges

ad1636d

address the comments and improve the excpetion handling logic

e314f62

vilyapilya changed the title ~~WIP:Optimize thread usage and handle http exceptions.~~ Optimize thread usage and handle http exceptions. Sep 5, 2019

dchai76 suggested changes Sep 5, 2019

View reviewed changes

lvilya added 3 commits September 5, 2019 16:41

add rubycop config file and run it

00d652c

change the oprand

25b7176

forgot about one more

ca85153

dchai76 suggested changes Sep 6, 2019

View reviewed changes

vilyapilya commented Sep 27, 2019

View reviewed changes

lvilya added 2 commits September 27, 2019 14:01

address comments

88f86cc

lints

78e24c6

dchai76 suggested changes Sep 27, 2019

View reviewed changes

change the locking logic ang change linting rules

a8c3ab1

lvilya added 2 commits October 9, 2019 11:01

rubocop

d8e4ad3

forgot to move another flag swtich into the lock block

690fa67

dchai76 reviewed Oct 10, 2019

View reviewed changes

lvilya added 3 commits October 10, 2019 14:27

restore the possibility to pass blocks as messages

86cc043

change the logic to read easier

9f86f72

update rubocop although it didd not have any effect. rubocop is stupid.

443f8d0

dchai76 suggested changes Oct 16, 2019

View reviewed changes

lvilya added 5 commits October 17, 2019 13:15

commit

cdb6ced

conflicts

1ecf5cb

sdfsdf

e66c44b

sdfsdF

4316bf4

fix the dep

5a2ecaf

dchai76 reviewed Oct 17, 2019

View reviewed changes

smusali requested review from smusali and removed request for respectus and smusali October 21, 2019 18:49

lvilya added 3 commits October 21, 2019 17:45

address the comments

1ac174b

remove the stray debug message

9d30f80

gitigone

40484a2

smusali requested a review from dchai76 October 24, 2019 22:00

dchai76 approved these changes Oct 24, 2019

View reviewed changes

remove the block

2de8895

dchai76 suggested changes Oct 25, 2019

View reviewed changes

.gitignore Outdated

*.gem

.DS_Store

/bin/

package-lock.json

Copy link

Contributor

dchai76 Oct 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack no, this is back. Remove.

gitingone

c9a07ff

vilyapilya merged commit ea5606b into master Oct 26, 2019

vilyapilya deleted the improve branch October 26, 2019 03:13

smusali mentioned this pull request Dec 17, 2019

Automate Ruby Code Library Release #13

Merged

Optimize thread usage and handle http exceptions. #7

Optimize thread usage and handle http exceptions. #7

Uh oh!

Conversation

vilyapilya commented Aug 22, 2019

Uh oh!

smusali left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dchai76 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dchai76 Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

dchai76 left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

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!

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

dchai76 Sep 4, 2019 •

edited

Loading