search

census-instrumentation / opencensus-go

A stats collection and distributed tracing framework
http://opencensus.io
Apache License 2.0
2.06k stars 327 forks source link

Defer IDGenerator initialization until first use #1228

Closed jlebon closed 4 years ago

jlebon commented 4 years ago

Initializing the IDGenerator from init() means that any downstream project which transitively imports this package somewhere in its dependency tree will incur getrandom() syscalls it has no control over at startup.

This causes problems for us in Ignition, where we're transitively pulling in this package via cloud.google.com/go/storage. Ignition runs very early during the boot process, which means that even though this isn't using GRND_RANDOM, the getrandom syscall can block until the entropy pool is ready. This is a real problem when running in VMs on systems which don't provide a virtualized RNG device (such as VMware) or which lack RDRAND.

I can't find a good reference for this, but I think in general it should be considered good practice to avoid I/O like this in init() in favour of a more lazy approach (or an explicit Initialize() function for clients to call).

Otherwise, every program which pulls in the package will pay for it, whether or not they intend to actually make use of the functionality those syscalls are priming. (While it's not relevant here, another advantage of not using init() for this is that I/O is fallible, and init() semantics means errors can't be handled sanely.)

Let's rework things here so that we don't actually initialize the IDGenerator fields until the first time it's used.

jlebon commented 4 years ago

Apologies for the drive-by contribution. For full disclosure, I only have a superficial understanding of this codebase and I'll admit that I haven't tested a trace with this patch. Unit tests do pass locally though.

nilebox commented 4 years ago

Thanks @jlebon for writing up the details of the issue and its impact! Please note that the OpenCensus project is in maintenance as it's getting replaced with the OpenTelemetry project, so we are only considering changes which address critical issues / use cases, and based on the description your PR matches that criteria.

Let me carefully review this change and get back to you in the next few days.

nilebox commented 4 years ago

@jlebon you may switch to a pinned commit for dependency or wait until we make a new release for opencensus-go.

nilebox commented 4 years ago

Release published: https://github.com/census-instrumentation/opencensus-go/releases/tag/v0.22.5