Skip to content

Conversation

t-umeno
Copy link
Contributor

@t-umeno t-umeno commented Aug 24, 2016

pmacct-1.5.2 uses template type 152 field (flowStartMilliseconds) and type 153 field(flowEndMilliseconds) instead of type 22 field (first_switched) and type 21 field (last_switched).

This patch supports:
150:flowStartSeconds
151:flowEndSeconds
152:flowStartMilliseconds
153:flowEndMilliseconds
154:flowStartMicroseconds
155:flowEndMicroseconds
156:flowStartNanoseconds
157:flowEndNanoseconds

This patch is based on:
https://github.com/logstash-plugins/logstash-codec-netflow/blob/master/lib/logstash/codecs/netflow/ipfix.yaml
https://github.com/logstash-plugins/logstash-codec-netflow/blob/master/lib/logstash/codecs/netflow.rb

flowStartSeconds
flowEndSeconds
flowStartMilliseconds
flowEndMilliseconds
flowStartMicroseconds
flowEndMicroseconds
flowStartNanoseconds
flowEndNanoseconds

Signed-off-by: Takashi Umeno <umeno.takashi@gmail.com>
Signed-off-by: Takashi Umeno <umeno.takashi@gmail.com>
format_for_flowSeconds(time)
format_for_flowMilliSeconds(time)
format_for_flowMicroSeconds(time)
format_for_flowNanoSeconds(time)

Signed-off-by: Takashi Umeno <umeno.takashi@gmail.com>
Signed-off-by: Takashi Umeno <umeno.takashi@gmail.com>
Signed-off-by: Takashi Umeno <umeno.takashi@gmail.com>
Signed-off-by: Takashi Umeno <umeno.takashi@gmail.com>
event['last_switched'] = format_for_switched(msec_from_boot_to_time(event['last_switched'], pdu.uptime, time, 0)) if event['last_switched']
r.each_pair do |k, v|
case k.to_s
when /^(?:first|last)_switched$/
Copy link
Owner

Choose a reason for hiding this comment

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

Could you check the performance to compare string and regexp?

  • string
case k
when 'first_switched'.freeze
when 'last_switched'.freeze
when 'flowStartSeconds'freeze
# ...
end
  • regexp
case k
when /^(?:first|last)_switched$/
when /^flow(?:Start|End)Seconds$/
# ...
end

Copy link
Contributor Author

@t-umeno t-umeno Aug 26, 2016

Choose a reason for hiding this comment

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

NetFlow-Generator genarates only NetFlow V5 packets.
https://github.com/mshindo/NetFlow-Generator

Please tell me how can I genarates NetFlow V9 test packets.
Otherwise I will make test program to check the performance to compare string and regexp.

@t-umeno
Copy link
Contributor Author

t-umeno commented Sep 1, 2016

$ ./benchmark3b.rb
0.460000 0.000000 0.460000 ( 0.539024)

#!/usr/bin/ruby
require 'benchmark'
print Benchmark.measure{ 1000000.times {
                           k = 'flowEndNanoseconds'
                           case k
                           when 'first_switched'.freeze then

                           when 'last_switched'.freeze then

                           when 'flowStartSeconds'.freeze then

                           when 'flowEndSeconds'.freeze then

                           when 'flowStartMilliseconds'.freeze then

                           when 'flowEndMilliseconds'.freeze then

                           when 'flowStartMicroseconds'.freeze then

                           when 'flowEndMicroseconds'.freeze then

                           when 'flowStartNanoseconds'.freeze then

                           when 'flowEndNanoseconds'.freeze then

                           else

                           end
                   }
}

@t-umeno
Copy link
Contributor Author

t-umeno commented Sep 1, 2016

$ ./benchmark4b.rb
1.360000 0.000000 1.360000 ( 1.551171)

#!/usr/bin/ruby
require 'benchmark'
print Benchmark.measure{ 1000000.times {
                           k = 'flowEndNanoseconds'
                           case k
                           when /^(?:first|last)_switched$/ then

                           when /^flow(?:Start|End)Seconds$/ then

                           when /^flow(?:Start|End)(Milli|Micro|Nano)seconds$/ then

                           else

                           end
                   }
}

@t-umeno
Copy link
Contributor Author

t-umeno commented Sep 1, 2016

So, I will rewrite with string.

Signed-off-by: Takashi Umeno <umeno.takashi@gmail.com>
event['first_switched'] = format_for_switched(msec_from_boot_to_time(event['first_switched'], pdu.uptime, time, 0)) if event['first_switched']
event['last_switched'] = format_for_switched(msec_from_boot_to_time(event['last_switched'], pdu.uptime, time, 0)) if event['last_switched']
r.each_pair do |k, v|
case k.to_s
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure why original implementation calls to_s, but key is string so no need to_s call here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why original implementation calls to_s, but key is string so no need to_s call here.

No!
case k.to_s is needed.
If you change case k,
'flowStartSeconds' doesn't match 'flowStartSeconds'.freeze.
Please check in your environment.

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm... it means 'k' is not string type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'k' is Symbol type, so case k.to_s is needed.

r.each_pair do |k, v|
$log.warn "k.class: ", k.class
$log.warn "k.to_s.class: ", k.to_s.class
$log.warn "'first_switched'.freeze.class: ", 'first_switched'.freeze.class
case k.to_s

2016-09-06 22:02:03 +0900 [warn]: k.class: Symbol
2016-09-06 22:02:03 +0900 [warn]: k.to_s.class: String
2016-09-06 22:02:03 +0900 [warn]: 'first_switched'.freeze.class: String

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry for the late.
I understood the situation.
Better way is key = k.to_s before case and refer it in case and [].
It reduces the number of to_s call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you.
I rewrote along your advice.

@t-umeno
Copy link
Contributor Author

t-umeno commented Oct 7, 2016

master change: from k.to_s to key.
#53.3 failed
https://travis-ci.org/repeatedly/fluent-plugin-netflow/jobs/165544056

I think this failure is not caused by this change.
(issue of class Rational?)
Do you have any idea to resolve this error?

@t-umeno
Copy link
Contributor Author

t-umeno commented Oct 25, 2016

Are there any problems in this patch to merge ?

@repeatedly
Copy link
Owner

Why you revert key = k.to_s code?

@repeatedly
Copy link
Owner

Another idea is using symbol for case and use k.to_s in [].

@repeatedly
Copy link
Owner

repeatedly commented Oct 26, 2016

case k
when :first_switched
  event[k.to_s] = ...
# ... other fields
end

Maybe, this is more faster than string case matching.

@t-umeno
Copy link
Contributor Author

t-umeno commented Oct 29, 2016

Thank you for you advice. I rewrite along this idea.

@repeatedly
Copy link
Owner

Thanks for the update.
I will check it later.

@repeatedly
Copy link
Owner

LGTM. Could you add a test?

@t-umeno
Copy link
Contributor Author

t-umeno commented Nov 3, 2016

Sorry, I can't add test codes immediately.

@t-umeno
Copy link
Contributor Author

t-umeno commented Nov 3, 2016

@t-umeno
Copy link
Contributor Author

t-umeno commented Nov 6, 2016

I add some test codes.
Please check these test codes.

@repeatedly repeatedly merged commit acb3c56 into repeatedly:master Nov 7, 2016
@repeatedly
Copy link
Owner

Thanks!

@t-umeno
Copy link
Contributor Author

t-umeno commented Nov 7, 2016

Thank you!

@t-umeno t-umeno deleted the flowStartMilliseconds branch November 7, 2016 14: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.

3 participants