Bug 1804711 - Add schedules_pings property to pings so that pings can be scheduled to be sent alongside other pings. #2791
Conversation
|
So at least some of these failures are due to the fact that the CI isn't using the glean_parser patch in mozilla/glean_parser#682. I'm not sure what to do about that. |
mikeconley
changed the title
glean-core/src/metrics/ping.rs
Outdated
| @@ -130,6 +138,10 @@ impl PingType { | |||
| self.0.include_info_sections | |||
| } | |||
|
|
|||
| pub(crate) fn schedules_pings(&self) -> &Vec<String> { | |||
There was a problem hiding this comment.
| pub(crate) fn schedules_pings(&self) -> &Vec<String> { | |
| pub(crate) fn schedules_pings(&self) -> &[String] { |
it's unusual to return a reference to a vec.
| ping.schedules_pings | ||
| ); | ||
| for scheduled_ping_name in &ping.schedules_pings { | ||
| glean.submit_ping_by_name(scheduled_ping_name, reason); |
There was a problem hiding this comment.
Hm, that means we also use the same reason. The reason for the actual submitted ping might not be specified for the linked ping.
We should at the very least document that related pings should use the same reasons (this might be done in a followup).
There was a problem hiding this comment.
That makes sense, if a custom ping is sharing an internal ping schedule, then the reason it was sent should match. I agree, we should document this behavior and probably might go a step further and either add a glean_parser lint or outright forbid pings that make use of internal ping schedules from declaring their own reasons.
That's fine, we can review the code and once we're happy we do the glean_parser release and pull that into this PR as well (it's an unfortunate shortcoming of the repo split) |
| ping.schedules_pings | ||
| ); | ||
| for scheduled_ping_name in &ping.schedules_pings { | ||
| glean.submit_ping_by_name(scheduled_ping_name, reason); |
There was a problem hiding this comment.
That makes sense, if a custom ping is sharing an internal ping schedule, then the reason it was sent should match. I agree, we should document this behavior and probably might go a step further and either add a glean_parser lint or outright forbid pings that make use of internal ping schedules from declaring their own reasons.
mikeconley
force-pushed
the
schedules_pings
branch
from
229ec8f
to
6c68c12
Compare
I ended up hitting a "too-many-arguments" clippy failure in metrics/ping.rs, but I figured it was a bit out of scope to change Glean's constructor signature for PingType to avoid this, so I added an allow.
…chedules_pings. While glean_parser will try to prevent the base-case of pings trying to schedule _themselves_ when being sent, nothing in this stack takes care of the problem of recursive cycles of pings. So if we have PingA scheduled to be sent when PingB is sent, and PingB is scheduled to be sent when PingC is sent, and PingC is scheduled to be sent when PingA is sent... things get messy indeed, and I think we might get stuck in a ping-sending loop.
mikeconley
force-pushed
the
schedules_pings
branch
from
6c68c12
to
452e315
Compare
|
Since this requires a little interaction in order to merge with the changes I was working on, and include an update to glean_parser, let's not land this PR directly. Instead, I've merged it into my PR #2792 and will include a commit in that stack that will update glean_parser in glean-core. I won't squash my PR to preserve the commit history post merge. Any objections or problems that anyone sees with that approach, please let me know. |
|
Merged as part of #2792 |
This is for https://bugzilla.mozilla.org/show_bug.cgi?id=1804711.
This relies on mozilla/glean_parser#682.
What this does it make it possible for a Glean ping to declare that it wants to be sent when another ping is sent. Supposing some product wanted to sent a ping as frequently as the baseline ping, then that ping could use the
ping_schedulemetadata property and listbaseline.This pull request is the part that takes the reverse-mapping that glean_parser creates, and actually does the lookup for other scheduled pings when a ping is sent.
I note this in one of the commit messages, but I think it's worth calling out here too:
While glean_parser will try to prevent the base-case of pings trying to schedule themselves when being sent, nothing in this stack takes care of the problem of recursive cycles of pings.
So if we have PingA scheduled to be sent when PingB is sent, and PingB is scheduled to be sent when PingC is sent, and PingC is scheduled to be sent when PingA is sent... things get messy indeed, and I think we might get stuck in a ping-sending loop.