1944586 - collection-disabled pings on artifact builds misbehave when testResetFOG is used

Status: RESOLVED FIXED

(Data Platform and Tools :: Glean: SDK, defect, P2)

Description

[Jan-Erik Rediger [:janerik]]

We should also add a smoke-test for collection-enabled pings and that server-knobs does not control them (that's at least the current implemented behavior).

In doing so however I encountered an issue that makes it fail when it shouldn't.

Comment 1

[Jan-Erik Rediger [:janerik]]

Attachment: Bug 1944586 - Reset JOG in tests as well r?chutten!

This test currently fails when it is run along with the full file.
I'm 99% sure it's because some state is left-over from the
testResetFog.

I think what's happening is that the JOG pings differ from the builtin
ones and we don't reset the state in the right places.
Note that this is only an issue for artifact builds and does not affect
full builds.

Comment 2

[Jan-Erik Rediger [:janerik]]

This turned from "server knobs for collection-disabled pings" to "collection-disabled pings in artifact builds and testResetFOG don't mix well".

Here's what I (think I) see, after adding extensive logging in Glean and FOG (grrrr, I need better debugging than print-debugging):

1. First FOG starts and registers all (statically) known pings. They have some ID.
2. Later JOG runs, loads all JOG pings and registers them. They get a new ID and get registered with Glean again (here)
3. Whether a ping is enabled is a bit complex now, but on metric recording we look up a ping by name in the registry and then check if it's enabled. For a ping name we only keep the latest registration
4. testResetFOG re-sets FOG/Glean and re-registers pings, but of course only those statically known then.
5. JOG pings do not get re-registered.
6. Calling set_enabled now might happen on the wrong instance of a ping. Then later writes target another instance and thus won't observe the same enabled value.

The following test reproduces the issue:

add_task(function test_fog_collection_disabled_pings_on_artifact_builds() {
  Services.fog.testResetFOG();

  GleanPings.collectionDisabledPing.setEnabled(true);
  Glean.testOnly.collectionDisabledCounter.add(3);
  Assert.equal(
    3,
    Glean.testOnly.collectionDisabledCounter.testGetValue()
  );
});

This test succeeds on a full build and fails on a artifact build.
(collectionDisabledPing is from bug 1943977).

I'm inclined to drop this bug. I'm currently not sure how to best fix it, there's a lot of state kept around.
So either unowning it and see if someone eventually picks it up or even close it as WONTFIX - collection-disabled pings on artifact builds will remain somewhat buggy.
(Pings don't carry other mutable state around so I'm not too worried about other codepaths hitting this issue)

:chutten, opinions?

Comment 3

[Chris H-C :chutten]

I think testResetFOG could call a newly-written testResetJOG which clears sFoundAndLoadedJogFile to Nothing() and then the first call to a Glean or GleanPings named getter would (on artifact builds) re-register all metrics and pings. That plus https://phabricator.services.mozilla.com/D235948 should make it work?

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b3bff1aba679
Reset JOG in tests as well r=TravisLong

Comment 5

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/b3bff1aba679