Create local Marshaler struct that replicates protobuf/jsonpb

[nleiva]

Related to issues/5. We add EmitUInt64Unquoted to jsonpb.Marshaler and replicated all its unexported methods.

type Marshaler struct {
	jsonpb.Marshaler
	// Whether to render int64/uint64 as strings or not?
	EmitUInt64Unquoted bool
}

This will let us simplify dependency management (current status is just a hack).

[remingtonc]

Thanks @nleiva. I was thinking about this, this morning. Would it be a better idea in the output stages e.g. metrics_influx to try and type data according to the types instead of explicitly build in the deserialization and replace that existing functionality? This only effects self-describing correct?

[nleiva]

This affects all data encoded with protobuf (compact and self-describing) when generating a JSON representation of it. Therefore changes are required on codec_gpb.go and codec_gnmi.go.

Unlike unit32, uint64 is represented as a string when producing a JSON output from protobuf. See JSON Mapping.

I'm good either way (you can remove this field as well and let the output remove the quotes to uint64 values), but this is a problem we need to address before making any further changes as we want to keep the dependencies clean.

[remingtonc]

@nleiva I agree it does suck that json and jsonpb aren't exactly compatible. I want to do a little investigating that pushes this responsibility to the output functions... This is a weird problem to have, and it looks like it was done explicitly due to JavaScript limitations which kind of sucks.

[remingtonc]

I'll test this as is and if it works I am fine to merge and follow up in another issue.

[nleiva]

It's up to you. I haven't done any functionality testing yet other than being able to compile the code, if it looks to you I think it will remove an issue from the current code.

The main objective of this pull request is to expose this "trick" in the code as we will need to address it sooner or later.

[remingtonc]

I fully agree and I like the patch removal and gets us closer to updating the codebase. Testing today.

[remingtonc]

Tested and it works. @sbyx given you started this repository, want to run the change by you.

[remingtonc]

Compiled via Docker, tested against live traffic, everything functional.

[remingtonc]

File deleted which is part of tests.

[sbyx]

Emitting strings for large numbers should be in accordance to RFC 7159 aka. JSON_IETF

[nleiva]

Emitting strings for large numbers should be in accordance to RFC 7159 aka. JSON_IETF

Right. I believe that's what Chris tried to do with this. See his comments "Paraphrased: while controversial, it is deemed safer to use string encoding for u/int64 to make sure that implementations using IEEE574 for numbers do not go wrong on numbers outside the 53 bits of integer precision they support. Hence their choice for 64 bit being a string in the mapping.
While we control consumers (e.g. no js consumers), we will marshal to numbers, so unmarshalling on the other side can be results in a comparable numeric without special case coercion.".

We are not changing anything about the output with this pull request, just making sure we do not depend on changes to the vendor folder. This folder will eventually go away (no longer needed with Go modules).

[remingtonc]

Fails in Docker build with:

Cloning into '/pipeline'...
Checking out files: 100% (9525/9525), done.
$GOPATH/go.mod exists but should not

Replication:

cd docker
docker build -t cisco-ie/pipeline-gnmi:6 .

Investigating.

[nleiva]

I'll take a look as well. At some point we also want to take a look at Multi-stage Docker builds to keep the image small and make this process way simpler!; Build from a Go ready image, then copy to an Alpine image without the need of all the package installation we have today. Look at this example.

I first thought the container would attempt to build this with Go 1.12.5 -> pkgs.alpinelinux.org:go, but the logs say 1.10.8 (Installing go (1.10.8-r0)).

The error reported was a consequence of 108379.

[remingtonc]

Rebased to current master, ran the tests, fixed the Dockerfile. Would prefer if someone else would verify as well. Rebased before #26 so make testall.