Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

Lazily initialize the view package's default worker #1287

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

brandur
Copy link

@brandur brandur commented Dec 11, 2022

See discussion in #1191 for impetus, but in general, it's not
particularly good practice to start up goroutines in package init
functions, with one reason being that it means that they're started for
projects that don't even make use of the package, and maybe just have it
as a transitive dependency.

So here, remove the view package's default worker in favor of a new
function that'll lazily initialize one when any package-level functions
are called that use the default worker. We use a sync.Once that has an
optimized short-circuit path in case the work's already been done, so
new overhead should be negligible, and registering/unregistering views
is probably not a particularly common operation anyway.

This change is backwards compatible, with use of the package's API
staying exactly the same. Test coverage seems to be pretty good, with a
function that previously reset the default worker between test cases
which I've modified to instead reset the default worker back to its
uninitialized state, and thereby verify that the test cases can
initialize it if necessary.

I thought about trying to deinitialize the default worker in case its
last view is unregistered, but decided against it because (1) it's more
invasive, (2) it's not likely to actually help anyone in practice as
views probably stay registered for a program's entire duration anwyay,
and (3) its interaction with the existing package-level Stop function
would take some thinking through.

See discussion in census-instrumentation#1191 for impetus, but in general, it's not
particularly good practice to start up goroutines in package `init`
functions, with one reason being that it means that they're started for
projects that don't even make use of the package, and maybe just have it
as a transitive dependency.

So here, remove the `view` package's default worker in favor of a new
function that'll lazily initialize one when any package-level functions
are called that use the default worker. We use a `sync.Once` that has an
optimized short-circuit path in case the work's already been done, so
new overhead should be negligible, and registering/unregistering views
is probably not a particularly common operation anyway.

This change is backwards compatible, with use of the package's API
staying exactly the same. Test coverage seems to be pretty good, with a
function that previously reset the default worker between test cases
which I've modified to instead reset the default worker back to its
uninitialized state, and thereby verify that the test cases can
initialize it if necessary.

I thought about trying to deinitialize the default worker in case its
last view is unregistered, but decided against it because (1) it's more
invasive, (2) it's not likely to actually help anyone in practice as
views probably stay registered for a program's entire duration anwyay,
and (3) its interaction with the existing package-level `Stop` function
would take some thinking through.
@brandur brandur requested review from a team and rghetia as code owners December 11, 2022 21:03
@google-cla
Copy link

google-cla bot commented Dec 11, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@brandur
Copy link
Author

brandur commented Dec 11, 2022

I noticed there seems to be a pre-existing intermittent test problem in the package which can be repro'ed on master with something like:

$ go test ./stats/view -run Test_Worker_MultiExport -count=20
--- FAIL: Test_Worker_MultiExport (0.00s)
    worker_test.go:217: Mismatched value (want 2, got 1) for &{[{a true}] [{2022-12-11 13:06:06.331908 -0800 PST m=+0.003086001 1}] 2022-12-11 13:06:06.331901 -0800 PST m=+0.003079585} in "/VF1"
    worker_test.go:222: Mismatched value (want 7.500000, got 2.000000) for &{[] [{2022-12-11 13:06:06.331908 -0800 PST m=+0.003086001 2}] 2022-12-11 13:06:06.331901 -0800 PST m=+0.003079585} in "/VF2"

I didn't take a stab at it though because I feel like someone else would be able to fix this a lot quicker.

@dashpole
Copy link
Collaborator

Fixing presubmit: #1288

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants