v0.40 new dependencies

[drakkan]

Hello,

in my application I use client_golang for instrumenting application code. If I update prometheus/common to v0.40 the increase in binary size is about 1 MB.

I see that there are some new dependencies (json-iterator, modern-go/reflect2, moder-go/concurrent), I haven't looked at the code so I'm not sure if they are needed for my use case. Unless strictly required, can you please avoid including them unconditionally (e.g. by not instantiating these methods in init or by creating a separate module)? Thank you

[roidelapluie]

cc @beorn7 Can you look at this? This seems histogram related.

[beorn7]

That's certainly #440. The init functions are in model/value_float.go and model/value_histogram.go . Not sure what's the best way to handle this. client_golang certainly needs the marshaling (for the API client), but whoever only uses the instrumentation library within client_golang shouldn't use it. Maybe avoiding the init functions will actually avoid linking in the dependencies, but what is a good way of doing so with json-iterator? I'm not very familiar with the latter.

[drakkan]

Thanks for considering this request. I use the library like this.
I'm not familiar with this library and so not sure what the best approach is, ideally only the used library should be linked.
Perhaps a build tag that we can optionally pass at compile time might be a good compromise. I do something similar for pgx for example

https://github.com/jackc/pgx/blob/master/pgtype/doc.go#L172

I build using the nopgxregisterdefaulttypes tag. Thank you

[beorn7]

I have tried to build a small program using the instrumentation library without the init functions in model/value_float.go and model/value_histogram.go. The binary size did not shrink. There must be another trick.

[drakkan]

I have tried to build a small program using the instrumentation library without the init functions in model/value_float.go and model/value_histogram.go. The binary size did not shrink. There must be another trick.

You are right. I can confirm. I supposed the compiler was able to figure outs that json-iterator/go is not used but this isn't the case.

I did a quick test with this patch that completely removes json-iterator/go

0001-proof-of-concept.zip

metrics in SFTPGo still seems ok

Am I missing something or does client_golang import a lot of unused methods?

[beorn7]

I mean, it imports common/model and actually uses it. With #440, it just became a whole lot heavier, and apparently, the Go linker acts on a package level, so you get the whole package, even the parts you aren't using.

[beorn7]

So here is my theory: After #440 , we started to use json-iterator for the first time at all in common. We used it before in client_golang, but only in the API client part. So only if you were actually using the latter, you would get json-iterator baked into your binary. After #440, json-iterator is used by common/model, and even without that init function, it has to be baked into your binary if you import common/model. Which is done by the instrumentation library in client_golang.

The API client in client_golang added marshaling with json-iterator to some types in common/model after the fact, for efficiency reasons. I guess we could do the same trick and move all the json-iterator marshaling into the API client within client_golang, just that is gets way more involved this time. Plus, whoever uses just common/model outside of client_golang doesn't get the benefits of json-iterator.

I see the following options:

1. Just leave it as is and accept the increased binary size (the example/random program grows from ~12MiB to ~14MiB, i.e. it's not a huge growth in relative terms, and bigger programs would hardly notice). If we go down that path, we should probably move the json-iterator stuff from the client_golang API client down into common/model for consistency.
2. We move all the json-iterator touching stuff from common/model up into the client_golang API client, essentially following the same strategy as before, just that it now appears way hackier because the marshaling for histograms is more complex.
3. The instrumentation library within client_golang is using only very few and light-weight parts of common/model. We could copy those over or move them into a separate package common/model/simple or something.

Any thoughts? I'm not perfectly happy with either.

I might hack up a pair of PRs for option (2) to see how bad it actually looks.

[drakkan]

So here is my theory: After #440 , we started to use json-iterator for the first time at all in common. We used it before in client_golang, but only in the API client part. So only if you were actually using the latter, you would get json-iterator baked into your binary. After #440, json-iterator is used by common/model, and even without that init function, it has to be baked into your binary if you import common/model. Which is done by the instrumentation library in client_golang.

The API client in client_golang added marshaling with json-iterator to some types in common/model after the fact, for efficiency reasons. I guess we could do the same trick and move all the json-iterator marshaling into the API client within client_golang, just that is gets way more involved this time. Plus, whoever uses just common/model outside of client_golang doesn't get the benefits of json-iterator.

I see the following options:

1. Just leave it as is and accept the increased binary size (the example/random program grows from ~12MiB to ~14MiB, i.e. it's not a huge growth in relative terms, and bigger programs would hardly notice). If we go down that path, we should probably move the json-iterator stuff from the client_golang API client down into common/model for consistency.
2. We move all the json-iterator touching stuff from common/model up into the client_golang API client, essentially following the same strategy as before, just that it now appears way hackier because the marshaling for histograms is more complex.
3. The instrumentation library within client_golang is using only very few and light-weight parts of common/model. We could copy those over or move them into a separate package common/model/simple or something.

Any thoughts? I'm not perfectly happy with either.

I might hack up a pair of PRs for option (2) to see how bad it actually looks.

Thank you for your analysis. If you create common/model/simple maybe common/model can just import common/model/simple and client_golang can import model/simple. Without duplications, maybe I'm missing something

[beorn7]

#453 and prometheus/client_golang#1225 should do the trick.

[drakkan]

#453 and prometheus/client_golang#1225 should do the trick.

I confirm. Thank you!!!