Bug 1943581 - Extend Glean's RLB PingType impl to support retrieving metadata #3055
Conversation
AdamBrouwersHarries
requested review from
travis79
and removed request for
a team
|
I've deliberately not added tests, as I'm a) not certain that this is the right API, and b) I don't know if it's something that would be acceptable to Glean devs. I'd rather not waste too much time writing tests for an API that gets rejected! |
There was a problem hiding this comment.
This looks okay to me, I'm not sure if you need all of this metadata or if you are just exposing everything that's available, but it would be nice to minimize this to just what you need, if possible. Aside from a couple of comments about comments (nits, all of them), I have no issue with moving forward with opening these up. Let's get a @badboy opinion on this too, before we land anything.
glean-core/rlb/src/private/ping.rs
Outdated
| @@ -94,6 +94,63 @@ impl PingType { | |||
| self.inner.submit(reason.map(|s| s.to_string())) | |||
| } | |||
|
|
|||
| /// Get the name of this Ping | |||
| /// Wrapper around glean::PingType::name | |||
There was a problem hiding this comment.
Since these would end up in rustdocs, I'd prefer that these function comments stand alone and not mention struct members from a separate crate.
glean-core/src/metrics/ping.rs
Outdated
| &self.0.name | ||
| } | ||
|
|
||
| pub(crate) fn include_client_id(&self) -> bool { | ||
| /// Whether the client ID will be included in the assembled ping. | ||
| /// See PingType::new for further details. |
There was a problem hiding this comment.
Since there aren't any additional details, you can omit the cross linking back to PingType::new, I think.
|
Just chatted with @badboy, no additional comments/concerns from him so if you don't mind making the comment changes and ensuring that the comment formats align with the rustdoc formatting, I'd be happy to approve and land this. Thanks! |
|
I don't really see a need for tests, the underlying Ping/PingType tests should cover this and these are just getters, so not really much to test unless there's some logic going on inside of the functions. |
|
Thanks for the feedback @travis79 ! I'll fix up the comments and re-push. |
|
Ah, I've just re-read the initial comment, and
I'm not entirely sure what we'll need. We want to be able to write a Gecko profiler marker when we submit a ping, but I'm not too certain what fields would be valuable. I've defaulted to everything "just in case", as I can't think of anything that we might not want. |
AdamBrouwersHarries
force-pushed
the
Bug1943581
branch
from
b1fbd1f to
dcf0c27
Compare
AdamBrouwersHarries
force-pushed
the
Bug1943581
branch
from
dcf0c27 to
548db4c
Compare
AdamBrouwersHarries
changed the title
No description provided.