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

Ensure tcp connection value is provided for all states, even when count is 0 #1329

Merged
merged 1 commit into from
Jul 13, 2020

Conversation

james-bebbington
Copy link
Member

Ensure tcp connection value is provided for all states, even when count is 0. Also update tests & improve test coverage.

@codecov
Copy link

codecov bot commented Jul 13, 2020

Codecov Report

Merging #1329 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1329      +/-   ##
==========================================
+ Coverage   89.60%   89.64%   +0.04%     
==========================================
  Files         215      215              
  Lines       15185    15197      +12     
==========================================
+ Hits        13606    13624      +18     
+ Misses       1151     1148       -3     
+ Partials      428      425       -3     
Impacted Files Coverage Δ
...internal/scraper/networkscraper/network_scraper.go 97.64% <100.00%> (+14.08%) ⬆️
translator/internaldata/resource_to_oc.go 81.39% <0.00%> (-4.66%) ⬇️

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 3711c01...b98a7d7. Read the comment docs.

func getConnectionStatusCounts(connections []net.ConnectionStat) map[string]int64 {
connectionStatuses := make(map[string]int64, len(connections))
func getTCPConnectionStatusCounts(connections []net.ConnectionStat) map[string]int64 {
var tcpStatuses = map[string]int64{
Copy link
Member

Choose a reason for hiding this comment

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

Why not an array and a enum for status? Seems better for me

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not totally sure what you mean by this.

Do you mean enum as in?

type tcpStatus string

const (
	closeWait   tcpStatus = "CLOSE_WAIT"
	closed      tcpStatus = "CLOSED"
	...
)

Also I think a map should outperform an array here since we need to iterate through potentially 100s of connections to record the counts. Using an array would require O(n^2)?

Copy link
Member

Choose a reason for hiding this comment

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

I mean an enum as in:

type tcpStatus int

const (
	closeWait   tcpStatus = iota
	closed      tcpStatus
	...
)

Copy link
Member

Choose a reason for hiding this comment

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

You may need a switch for the status. Not 100% sure so your call

Copy link
Member Author

@james-bebbington james-bebbington Jul 13, 2020

Choose a reason for hiding this comment

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

Oh okay yea - ofc 🤦‍♂️

We would need a switch to convert strings to indexes, so I suspect performance would be pretty similar either way. Annoyingly gopsutil actually converts these from ints to strings before returning the values 😞

If you don't feel too strongly about this I will probably leave it as is. This has the arguably useful propery that if we ever receive an unexpected status value, it will get added to the map.

@bogdandrutu bogdandrutu merged commit cd2586c into open-telemetry:master Jul 13, 2020
wyTrivail pushed a commit to mxiamxia/opentelemetry-collector that referenced this pull request Jul 13, 2020
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024
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