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

Goflow parse #361

Merged
merged 5 commits into from
Jul 15, 2021
Merged

Goflow parse #361

merged 5 commits into from
Jul 15, 2021

Conversation

jsirianni
Copy link
Member

@jsirianni jsirianni commented Jul 15, 2021

Description of Changes

  • Switched to observiq fork of Goflow
    • This fork sets a boolean value if "FlowDirection / BiFlowDirection" is received from the netflow source. This is because 0 is a valid value, and the json parser was filtering out zeros due to the omit empty tag. We want to skip this field if it was not sent from the netflow client.
  • Replaced structs.Map() call (converting goflow message to map[string]interface{} in favor of manual parsing.
    • This resolves an issue where proto field is missing when 0 (0 == ICMP)
  • Moved proto_name parsing from parse.go to parse_protocol.go
  • Parse() accepts pointer value to prevent copying mutex lock

This new behavior means test cases need to set the following to their zero values:

				"proto":          0,
				"proto_name":     "HOPOPT",
				"inif":           0,
				"outif":          0,

Proto, InIf, OutIf are always present (from what I have seen in the field). If they are not present, the user will see zero values and the proto_name "HOPOPT". It is possible a future version of the Goflow package will allow us to detect if the fields were sent from the client.

I observed significant performance increase

Master:

go test ./operator/builtin/input/goflow -bench=. -benchtime=10s

goos: linux
goarch: amd64
pkg: github.com/observiq/stanza/operator/builtin/input/goflow
cpu: AMD Ryzen 9 3950X 16-Core Processor       
     
BenchmarkParse-32    	  102042	    118210 ns/op
BenchmarkParse-32    	   96640	    117744 ns/op
BenchmarkParse-32    	  101474	    116649 ns/op

PASS

ok  	github.com/observiq/stanza/operator/builtin/input/goflow	13.263s

This PR:

go test ./operator/builtin/input/goflow -bench=. -benchtime=10s

goos: linux
goarch: amd64
pkg: github.com/observiq/stanza/operator/builtin/input/goflow
cpu: AMD Ryzen 9 3950X 16-Core Processor            

BenchmarkParse-32    	  971654	     12851 ns/op
BenchmarkParse-32    	 1000000	     12928 ns/op
BenchmarkParse-32    	 1000000	     12941 ns/op

PASS

ok  	github.com/observiq/stanza/operator/builtin/input/goflow	12.621s

Please check that the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • Add a changelog entry (for non-trivial bug fixes / features)
  • CI passes

jsirianni added 2 commits July 14, 2021 18:35
…if, outif which resolves a bug where proto 0 (ICMP) is dropped from the entry
@jsirianni jsirianni closed this Jul 15, 2021
@jsirianni jsirianni reopened this Jul 15, 2021
@jsirianni jsirianni closed this Jul 15, 2021
@djaglowski
Copy link
Member

Log Files Logs / Second CPU Avg (%) CPU Avg Δ (%) Memory Avg (MB) Memory Avg Δ (MB)
1 1000 1.6551741 +0.22406816 128.74258 -1.8078003
1 5000 5.0173345 -0.31027317 136.39964 -0.2883911
1 10000 9.982939 +0.051854134 146.44894 -3.0464783
1 50000 48.277054 -1.8439255 171.36153 -7.477646
1 100000 95.431564 -1.9628143 226.53435 -1.8065796
10 100 2.1553032 +0.27589774 134.56923 -0.55766296
10 500 5.7932997 -0.3792057 140.03596 -0.6356354
10 1000 11.414141 +0.017298698 147.43669 -0.8504791
10 5000 53.864235 -0.28815842 172.82785 -5.1876373
10 10000 103.494804 -1.9001007 214.06627 -8.561554

@jsirianni jsirianni reopened this Jul 15, 2021
@jsirianni jsirianni requested a review from djaglowski July 15, 2021 14:56
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

LGTM

@djaglowski
Copy link
Member

Log Files Logs / Second CPU Avg (%) CPU Avg Δ (%) Memory Avg (MB) Memory Avg Δ (MB)
1 1000 1.4310849 -2.1100044e-05 128.98856 -1.5618286
1 5000 5.172461 -0.1551466 137.84093 +1.1528931
1 10000 10.5349045 +0.60381985 146.47858 -3.0168457
1 50000 49.379807 -0.7411728 174.90907 -3.9300995
1 100000 95.82563 -1.5687485 233.09267 +4.7517395
10 100 1.7931755 -0.08623004 133.53085 -1.5960388
10 500 6.672552 +0.50004673 137.78569 -2.88591
10 1000 12.362235 +0.96539307 146.78812 -1.499054
10 5000 58.222443 +4.0700493 177.26292 -0.7525635
10 10000 105.724365 +0.32946014 225.91136 +3.2835388

@codecov
Copy link

codecov bot commented Jul 15, 2021

Codecov Report

Merging #361 (c2f90e4) into master (7dd3bae) will increase coverage by 0.11%.
The diff coverage is 95.49%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #361      +/-   ##
==========================================
+ Coverage   73.16%   73.26%   +0.11%     
==========================================
  Files         123      124       +1     
  Lines        7860     7947      +87     
==========================================
+ Hits         5750     5822      +72     
- Misses       1623     1633      +10     
- Partials      487      492       +5     
Impacted Files Coverage Δ
operator/builtin/input/goflow/goflow.go 29.55% <0.00%> (ø)
operator/builtin/input/goflow/parse.go 86.76% <86.05%> (-11.47%) ⬇️
operator/builtin/input/goflow/parse_protocol.go 100.00% <100.00%> (ø)
operator/builtin/input/tcp/tcp.go 72.66% <0.00%> (-3.91%) ⬇️
operator/builtin/output/forward/forward.go 60.49% <0.00%> (-1.23%) ⬇️
operator/builtin/input/file/file.go 80.63% <0.00%> (+1.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7dd3bae...c2f90e4. Read the comment docs.

@jsirianni jsirianni merged commit 88cae68 into master Jul 15, 2021
@jsirianni jsirianni deleted the goflow-parse branch July 15, 2021 15:08
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.

2 participants