Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New method to post UDP events to the stream with Ruby #5

Merged
merged 17 commits into from
Mar 14, 2014
Merged

Conversation

adrienDog
Copy link

Documentation updated using Yard.
2 tests created in statsd_spec.
Refs: #1

Was greatly inspired by the python version.

Documentation updated using Yard.
2 tests created in statsd_spec.
Refs: #1
it "Test with several tags" do
@statsd.event('Title_test', 'Text_test', :tags => %w(tag_test_1 tag_test_2))
@statsd.socket.recv.must_equal ['_e{10,9}:Title_test|Text_test|#tag_test_1,tag_test_2']
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see some more tests around the behavior added of other flags, to ensure that the addition or omission, ordering, etc behaves as expected.

@miketheman
Copy link
Contributor

Overall, it looks like string is being rewritten multiple times as you flow through the code, building up a new object each time, leading to memory bloat.

[1] pry(main)> string = "foo"
=> "foo"
[2] pry(main)> string.object_id
=> 70154227837960
[3] pry(main)> string = "#{string}|bar"
=> "foo|bar"
[4] pry(main)> string.object_id
=> 70154235031700

You could use the shovel operator to append to the existing object, like so:

[1] pry(main)> string = "foo"
=> "foo"
[2] pry(main)> string.object_id
=> 70270745414420
[3] pry(main)> string << '|bar'
=> "foo|bar"
[4] pry(main)> string.object_id
=> 70270745414420

Another cleanup is for any single-line if/end statements, you can put the evaluation inline with the code, like so:

date_happened = opts[:date_happened] || nil
string << "|d:#{date_happened}" if date_happened

@miketheman
Copy link
Contributor

Final note:
using string as the main variable to pass to the socket might be easily mistaken for String, which is the top-level object class.
Naming it to something like event_message or whatever in this code could prevent future mistakes, as well as declare what the purpose of the object is.

More testing in Spec and cleaner code
Refs: #5
@adrienDog
Copy link
Author

So I have made more tests, I don't know if they are sufficient.
I left it possible to change the hostname for an event, since it's available in the python file.

All modifications asked should be done I hope :)

Easier and cleaner to add parameters if needed
Refs: #5
@adrienDog
Copy link
Author

I have created a dictionary to link parameter names to their respective 1-letter key so that in the future it's faster to add more parameters.

Had to add faker to Gemfile and update he helper
Refs: #5
@miketheman
Copy link
Contributor

@adrienDog This is looking better and better.

As you can see, the tests failed. :) For travis to not install the yard/redcarpet (which it doesn't need), you can exclude them in .travis.yml.

For the tests, see the failures: most of them are due to the packet size being over 8K - so consider this:
Instead of running the entire test suite through 100 iterations of potential failures due to too much randomness, have a test that confirms the correct failure mode when the size is too large, and have the other tests behave correctly when the payload size is correct.

All in all, I like the way this is going, keep it up!

@adrienDog
Copy link
Author

Ok thanks Mike I will work on that !

@@ -0,0 +1,45 @@
# Load the dogstats module.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could rename this file "test-statsd.rb" (http://www.deathoftheauthor.com/).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this file was mistakenly added and committed. @adrienDog ?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah @miketheman it doesn't really need to be there. But it was a way for me to really send events to my localhost.
Do you think I should leave it there and rename it ? Or just not commit it ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's good to have an example handy to test, so I would clean it up, place it in an examples folder, with notes on why and when it can be used.

Copy link
Author

Choose a reason for hiding this comment

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

Ok I created the folder examples, renamed the test file in it more accurately I think.
I hope everything is fine now

@adrienDog
Copy link
Author

Yeah! Travis is my friend again

'aggregation_key',
'priority',
'source_type_name',
'alert_type',
Copy link
Contributor

Choose a reason for hiding this comment

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

This defines an array of the same elements that are represented in the above hash.
Is there any danger in sorting the payload's opt_keys before passing to .each instead?

'alert_type' => 't'
}
# We construct the string to be sent by adding '|key:value' parts to it when needed
opt_keys.sort_by { |name, key| key }.each do |name, key|

Choose a reason for hiding this comment

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

ops_keys never changes, so it should be a constant defined once in the global scope, instead of each time this function is called. Also, you're defining a hash which is unordered, and sorting it each time, which is wasteful. You could instead just have an array of arrays and avoid resorting:

OPTS_KEYS = [
      ['date_happened', 'd'],
      ['hostname', 'h'],
      ['aggregation_key', 'k'],
      ['priority', 'p'],
      ['source_type_name', 's'],
      ['alert_type', 't']
]

@adrienDog
Copy link
Author

@clofresh Is it better like this ?

adrienDog pushed a commit that referenced this pull request Mar 14, 2014
New method to post UDP events to the stream with Ruby. Tests have been updated. An example file s available in /examples
@adrienDog adrienDog merged commit 4dbc17e into master Mar 14, 2014
@albertvaka albertvaka deleted the events branch June 19, 2019 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants