Eliminate protobufs

[pboothe]

If we don't need them, then we should not keep them around. They will exist forever in our hearts and in our code history.

This also moves some code around, because there is important and good code in nl-proto/pbtools and nl-proto we need to save. This also increases code coverage by adding more tests for more packages.

---

This change is 

[coveralls]

Pull Request Test Coverage Report for Build 453

• 57 of 67 (85.07%) changed or added relevant lines in 7 files are covered.
• 2 unchanged lines in 1 file lost coverage.
• Overall coverage decreased (-0.6%) to 82.325%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
inetdiag/inetdiag.go	30	40	75.0%

Files with Coverage Reduction	New Missed Lines	%
inetdiag/inetdiag.go	2	91.57%

Totals
Change from base Build 429:	-0.6%
Covered Lines:	517
Relevant Lines:	628

---

💛 - Coveralls

[pboothe]

There's some complication around the fact that the TcpInfo in syscall is not as complete as the one we had in nl-proto

What is the right thing? I suspect we should use our own, larger struct.

[gfr10598]

Shouldn't matter if we aren't parsing it?

…

On Mon, Mar 25, 2019, 6:24 PM Peter Boothe ***@***.***> wrote: There's some complication around the fact that the TcpInfo in syscall is not as complete as the one we had in nl-proto What is the right thing? I suspect we should use our own, larger struct. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#54 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AT8mervLz08jd41ErkBXZogBgku6mC15ks5vaUyugaJpZM4cJJ1i> .

[pboothe]

I think you are right. PTAL.

Reviewable status: 0 of 1 LGTMs obtained

[gfr10598]

Reviewed 3 of 17 files at r2, 1 of 7 files at r3.
Reviewable status: 0 of 1 LGTMs obtained

---

.travis.yml, line 103 at r3 (raw file):

# Docker build
#- docker build --build-arg COMMIT=$TRAVIS_BRANCH -t mlab/tcpinfo .

We should keep the docker build until we can fail based on dockerhub build.

---

.travis.yml, line 106 at r3 (raw file):

#################################################################################
# Deployment Section

I'd rather keep this section commenting on deployments, even though we don't have any yet.

---

Dockerfile, line 11 at r3 (raw file):

# Build tcp-info
FROM golang:1.12.1-stretch as go-builder

Add comment about why stretch and why this version.

---

Dockerfile, line 25 at r3 (raw file):

# Build the image containing both binaries.
FROM alpine

Have you confirmed that this works? Or did you figure out how to static link?

---

inetdiag/inetdiag.go, line 43 at r3 (raw file):

	"unsafe"

	"github.com/m-lab/tcp-info/tcp"

Please keep our mlab imports separate at the bottom.

---

inetdiag/inetdiag.go, line 250 at r3 (raw file):

}

// ChangeType indicates why a new record is worthwhile saving.

FYI, I think I'd like to move this, but fine here for now.

---

inetdiag/inetdiag.go, line 287 at r3 (raw file):

// and CAState fields, these are probably adequate, but we also check for new or missing attributes
// and any attribute difference outside of the TCPInfo (INET_DIAG_INFO) attribute.
// TODO:

This TODO seems obsolete?

---

metrics/metrics.go, line 93 at r3 (raw file):

func init() {
	log.Println("Prometheus metrics in tcp-info.metrics are registered.")

auto-registered?

[gfr10598]

Reviewed 2 of 2 files at r1, 9 of 17 files at r2, 6 of 7 files at r3.
Reviewable status: 0 of 1 LGTMs obtained

---

main.go, line 74 at r3 (raw file):

	flagx.ArgsFromEnv(flag.CommandLine)

	if *outputDir != "" {

Should we mkdir -p ?

[pboothe]

PTAL

Reviewable status: 0 of 1 LGTMs obtained

---

.travis.yml, line 103 at r3 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

We should keep the docker build until we can fail based on dockerhub build.

Done.

---

.travis.yml, line 106 at r3 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

I'd rather keep this section commenting on deployments, even though we don't have any yet.

Done.

---

Dockerfile, line 11 at r3 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

Add comment about why stretch and why this version.

Reduced it to 1.12. The ".1-stretch" was vestigial.

---

Dockerfile, line 25 at r3 (raw file):
I have confirmed that

make MOREFLAGS="-static"

causes the binary to be built with static linking and allows me to copy the binary from ubuntu to alpine and have it successfully compress files.

---

main.go, line 74 at r3 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

Should we mkdir -p ?

No. The output directory should already exist. We make its subdirectories.

---

inetdiag/inetdiag.go, line 43 at r3 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

Please keep our mlab imports separate at the bottom.

Done.

---

inetdiag/inetdiag.go, line 250 at r3 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

FYI, I think I'd like to move this, but fine here for now.

SGTM. Added a TODO.

---

inetdiag/inetdiag.go, line 287 at r3 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

This TODO seems obsolete?

Deleted.

---

metrics/metrics.go, line 93 at r3 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

auto-registered?

Added explanatory comment.

[gfr10598]

Reviewed 4 of 4 files at r4.
Reviewable status: complete! 1 of 1 LGTMs obtained

---

Dockerfile, line 25 at r3 (raw file):

Previously, pboothe (Peter Boothe) wrote…

I have confirmed that

make MOREFLAGS="-static"

causes the binary to be built with static linking and allows me to copy the binary from ubuntu to alpine and have it successfully compress files.

Excellent! Thanks!

---

inetdiag/inetdiag.go, line 48 at r4 (raw file):

)

// TODO: Refactor this package, or at least this file. It feels like it

BTW, I agree!