Regexp tidy up

[jimmidyson]

Cleaned up some areas where regexps were either messy or unnecessary.

[timstclair]

Why move this out of the function? Personally I find it more readable if I can see the regexp right where it's used (and I don't think this should be used outside that function).

[jimmidyson]

Putting it in the function means the RE gets wastefully compiled every time the function is called. In this case probably not called more than once, but normally best to move it outside so trying to keep it consistent.

[timstclair]

Ah, makes sense. Maybe move the declaration to just above the function in that case? Also, consider making the string const.

[timstclair]

(nit) space after //

[timstclair]

Also this comment reads a little weird. Maybe rephrase to:

// TCP state is the 4th field.
// Format: sl local_address rem_address st tx_queue rx_queue tr tm->when retrnsmt  uid timeout inode

[jimmidyson]

@timstclair Thanks for a thorough code review - this is making it a much more worthwhile endeavour than it would have otherwise been :)

[timstclair]

This regex should use a raw string too.

[jimmidyson]

Done.

[timstclair]

@timstclair Thanks for a thorough code review - this is making it a much more worthwhile endeavour than it would have otherwise been :)

No problem! Thanks for the cleanup!

[jimmidyson]

All comments addressed. Good review!

[jimmidyson]

As an observation there is a lot we could do to lower resource utilisation. Regexps are hungry & need to be used judiciously. They're overused here where we could probably do some cheaper string parsing.

[jimmidyson]

Integration tests pass, so it must be good right? 😜

[timstclair]

This whole loop could use some cleanup. I think it would be much more efficient and readable to switch on strings.TrimSpace(line[:strings.Index(line, ":")-1])

That's sort of a separate change, so up to you where / whether you want to address it.

[jimmidyson]

Agreed. Not going to do it here, luckily AFAICT this is only called once on start of cadvisor so efficiency isn't paramount. Would like to improve it though, for vanity more than anything else :-b

[timstclair]

LGTM (please squash)

Integration tests pass, so it must be good right?

Of course! I always like to have some simple unit tests for regexes, but I won't force you to add them if you aren't up for it...

As an observation there is a lot we could do to lower resource utilisation. Regexps are hungry & need to be used judiciously. They're overused here where we could probably do some cheaper string parsing.

Agreed. Although I don't think it's worth spending too much time on this without first doing some profiling to identify the places it really matters.

[jimmidyson]

Yeah we need to do more profiling of cadvisor in general. There have been quite a few performance issues when embedded in the kubelet & we need to identify areas for improvement. Regexes were a recurrent theme in those perf problems, hence me looking at them here. I think with the work we've done recently in cadvisor this is improved significantly, but further work needs to be done to keep cadvisor's footprint as low as possible.

[jimmidyson]

retest this please

[jimmidyson]

Self-merging after LGTM.