1596420 - Provide better tooling for removing expired telemetry probes

Status: NEW

(Toolkit :: Telemetry, enhancement, P3)

Description

[(no longer active)]

We rely on email people/email lists when telemetry probes are about to expire, and it seems that we've been assuming that this process will work well somehow. I believe toolkit/components/telemetry/Histograms.json shows the contrary, that file is full of expired probes (some of which don't even have the collection code around any more) and it's up to anyone's guess for each one why the probe is expired and is left to rot.

I don't really blame anyone for the current system not working; while I've personally tried to react to these emails responsibly, I miss emails all the time, and it's also unclear what the incentives for removing a probe that's about to expire are.

It would be nice to think of a more realistic mechanism that can keep Histogram.json as a list of non-expired probes, since the current situation isn't really sustainable in the long term.

Comment 1

[Chris H-C :chutten]

Speaking from the Telemetry POV (the Glean SDK has different capabilities), I can see a few options.

1. We could fail the build whenever there's even a single expired probe present in any definitions file.
   • Pros: Keeps everything clean.
   • Cons: Renders version increase simulation builds ineffective at catching actual problems. Makes "there's an expired probe" the problem of the person building at that moment, not the problem of the person who owns the probe.
2. We could move expired probes to Histograms_expired.json (and Scalars_expired.yaml, etc.). The parsers and code generators know how to handle multiple source files (or can be augmented to do so).
   • Pros: Keeps Histograms.json clean.
   • Cons: Basically just pulls a Patrick by just putting them somewhere else. Since Histograms.json being clean of expired probes shouldn't necessarily be the goal in and of itself (only a byproduct of fixing the "we have a lot of expired Histograms in the tree" problem), it's questionable if this is actually worth it.
3. Abandon Histogram.json to its messy state and focus instead on making expired collections cheaper.
   • Pros: Minimizes effect on performance. Is achievable.
   • Cons: Doesn't feel like we're tackling the right problem. Also, I've looked into this, and the juice doesn't seem worth the squeeze: they're already very cheap.

None of the options I can come up with are very good. Ehsan, do you have any ideas of how we might proceed?

Comment 2

[(no longer active)]

(In reply to Chris H-C :chutten from comment #1)

Speaking from the Telemetry POV (the Glean SDK has different capabilities), I can see a few options.

1. We could fail the build whenever there's even a single expired probe present in any definitions file.
   • Pros: Keeps everything clean.
   • Cons: Renders version increase simulation builds ineffective at catching actual problems. Makes "there's an expired probe" the problem of the person building at that moment, not the problem of the person who owns the probe.

Not necessarily, IMHO it makes it possible for us to detect the problems in advance, get bugs filed and have people responsible to get the tree back to a green state. We do this for tests already (see for example bug 1596099 which coincidentally is an example of such a bug that I happen to be on the hook for today...)

2. We could move expired probes to Histograms_expired.json (and Scalars_expired.yaml, etc.). The parsers and code generators know how to handle multiple source files (or can be augmented to do so).
   • Pros: Keeps Histograms.json clean.

Yay!

 * Cons: Basically just [pulls a Patrick](https://knowyourmeme.com/memes/push-it-somewhere-else-patrick) by just putting them somewhere else.

Slight improvement over status quo, but yeah. I suppose if we can't get (1) then I could be forced to think this is a good option (but that's the stockholm syndrome speaking probably). :-)

Since Histograms.json being clean of expired probes shouldn't necessarily be the goal in and of itself (only a byproduct of fixing the "we have a lot of expired Histograms in the tree" problem), it's questionable if this is actually worth it.

Agreed.

3. Abandon Histogram.json to its messy state and focus instead on making expired collections cheaper.

Cheaper how? I mean we have actual code running in people's actual browsing code paths collecting and reporting on these probes which we then throw into the trash and don't submit anything into the telemetry ping for.

* Pros: Minimizes effect on performance. Is achievable.

I don't see how this is true but perhaps you're talking about the performance of another system (like somewhere in telemetry)?

* Cons: Doesn't feel like we're tackling the right problem. Also, I've looked into this, and [the juice doesn't seem worth the squeeze](https://bugzilla.mozilla.org/#c11): they're already very cheap.

Wrong link? But I do agree with your conclusion here nonetheless.

None of the options I can come up with are very good. Ehsan, do you have any ideas of how we might proceed?

Yes, option (1) sounds great to me.

Of course on top of that we need to figure out a way to deal with the existing sea of expired probes... So maybe option (2) to deal with the existing badness (contain the damage) and option (1) to prevent new spills?

Comment 3

[(no longer active)]

Oh I should mention that with option 1 I think we should break a test not necessarily break the build. The latter prevents the beta simulation jobs the sheriffs push from working at all, whereas the former integrates well with the existing simulation process.

Comment 4

[Chris H-C :chutten]

(In reply to :ehsan akhgari from comment #2)

(In reply to Chris H-C :chutten from comment #1)

Speaking from the Telemetry POV (the Glean SDK has different capabilities), I can see a few options.

1. We could fail the build whenever there's even a single expired probe present in any definitions file.
   • Pros: Keeps everything clean.
   • Cons: Renders version increase simulation builds ineffective at catching actual problems. Makes "there's an expired probe" the problem of the person building at that moment, not the problem of the person who owns the probe.

Not necessarily, IMHO it makes it possible for us to detect the problems in advance, get bugs filed and have people responsible to get the tree back to a green state. We do this for tests already (see for example bug 1596099 which coincidentally is an example of such a bug that I happen to be on the hook for today...)

Taking this with your Comment #3: Hm... Having it be a test would be best, but the only suite that has the necessary information would be the python tests in toolkit/components/telemetry/tests/python and as far as I know they break the build when they break (we use this to catch people not adhering to definitions rules like using too many buckets in their histograms and so forth).

We could build a test based on the information we have after a successful build (the expiry versions are present in the in-memory info structs, so we should be able to check this in, say, a gtest if we expose them), but I would like to keep all these sorts of "definitions files compliance tests" in the same place. I'd love to hear your thoughts on that.

(( Ultimately we could teach the version increase simulation build about strict_type_checks which, when false allows all sorts of nonsense to be parsed from the definitions files into the build. (this is used for historic compatibility) ))

3. Abandon Histogram.json to its messy state and focus instead on making expired collections cheaper.

Cheaper how? I mean we have actual code running in people's actual browsing code paths collecting and reporting on these probes which we then throw into the trash and don't submit anything into the telemetry ping for.

Cheaper as in binary size and memory size. If I'd provided the correct link (this one), then that might have helped :S

Each Histogram contributes bytes to the binary proportional to the size of HistogramInfo plus the length of their name. Expired histograms are never given other storage, but enough HistogramInfos add up.

None of the options I can come up with are very good. Ehsan, do you have any ideas of how we might proceed?

Yes, option (1) sounds great to me.

Of course on top of that we need to figure out a way to deal with the existing sea of expired probes... So maybe option (2) to deal with the existing badness (contain the damage) and option (1) to prevent new spills?

Well for that we've previously opted for grandfathering them in by putting any presently-non-conformant probe definitions in one of our many allowlists. It's how we deprecated the "count" Histogram kind when we came up with the much smaller, much easier to analyse "uint" Scalar kind.

Comment 5

[(no longer active)]

(In reply to Chris H-C :chutten from comment #4)

Not necessarily, IMHO it makes it possible for us to detect the problems in advance, get bugs filed and have people responsible to get the tree back to a green state. We do this for tests already (see for example bug 1596099 which coincidentally is an example of such a bug that I happen to be on the hook for today...)

Taking this with your Comment #3: Hm... Having it be a test would be best, but the only suite that has the necessary information would be the python tests in toolkit/components/telemetry/tests/python and as far as I know they break the build when they break (we use this to catch people not adhering to definitions rules like using too many buckets in their histograms and so forth).

Ugh.

We could build a test based on the information we have after a successful build (the expiry versions are present in the in-memory info structs, so we should be able to check this in, say, a gtest if we expose them), but I would like to keep all these sorts of "definitions files compliance tests" in the same place. I'd love to hear your thoughts on that.

A gtest could #include something like objdir/.../Histogram-expiry-info.h, which is autogenerated from Histograms.json. Then the test itself could be a small boilerplate that nobody really needs to look at, so it wouldn't matter where it is. Would that work?

(( Ultimately we could teach the version increase simulation build about strict_type_checks which, when false allows all sorts of nonsense to be parsed from the definitions files into the build. (this is used for historic compatibility) ))

This is true.

3. Abandon Histogram.json to its messy state and focus instead on making expired collections cheaper.

Cheaper how? I mean we have actual code running in people's actual browsing code paths collecting and reporting on these probes which we then throw into the trash and don't submit anything into the telemetry ping for.

Cheaper as in binary size and memory size. If I'd provided the correct link (this one), then that might have helped :S

Each Histogram contributes bytes to the binary proportional to the size of HistogramInfo plus the length of their name. Expired histograms are never given other storage, but enough HistogramInfos add up.

Ugh. So I guess this may actually be somewhat important for our memory usage in a post-Fission world?

None of the options I can come up with are very good. Ehsan, do you have any ideas of how we might proceed?

Yes, option (1) sounds great to me.

Of course on top of that we need to figure out a way to deal with the existing sea of expired probes... So maybe option (2) to deal with the existing badness (contain the damage) and option (1) to prevent new spills?

Well for that we've previously opted for grandfathering them in by putting any presently-non-conformant probe definitions in one of our many allowlists. It's how we deprecated the "count" Histogram kind when we came up with the much smaller, much easier to analyse "uint" Scalar kind.

Sounds like a great option, thanks for the suggestion!

Comment 6

[Chris H-C :chutten]

(In reply to :ehsan akhgari from comment #5)

(In reply to Chris H-C :chutten from comment #4)

We could build a test based on the information we have after a successful build (the expiry versions are present in the in-memory info structs, so we should be able to check this in, say, a gtest if we expose them), but I would like to keep all these sorts of "definitions files compliance tests" in the same place. I'd love to hear your thoughts on that.

A gtest could #include something like objdir/.../Histogram-expiry-info.h, which is autogenerated from Histograms.json. Then the test itself could be a small boilerplate that nobody really needs to look at, so it wouldn't matter where it is. Would that work?

We do something like that already with {Scalar|Event}ArtifactDefinitions.json where we read the yaml files and spit out json for consumption by JS. If we spit out an array of expired, non-allowed histogram names then the test needs only assert that the array isn't empty. It could work.

Each Histogram contributes bytes to the binary proportional to the size of HistogramInfo plus the length of their name. Expired histograms are never given other storage, but enough HistogramInfos add up.

Ugh. So I guess this may actually be somewhat important for our memory usage in a post-Fission world?

Yes and no. They're pretty cheap, and there aren't that many of them. Of course, they're 100% waste from people not cleaning up after themselves, so if we need an extra few kb then these are an excellent choice.

---

To sum up, we should set up a test that fails when there are any expired probes in-tree. This can tie into the version increase simulation process (+Cc Aryx, with whom I had a short offline conversation about how this might work) in a way fairly similar to the way that existing tests that fail because their telemetry expires do.

From a triage note, it is unlikely that we'll have the bandwidth to address this before December when the next cycle begins. But hopefully in time for January