New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug 1839640 - Add new metric type: object #2489
Conversation
There's still a couple of things I need to get in order for this to land, but it's good for a first review. |
24558ef
to
28830ef
Compare
6537fa2
to
4a8ac5e
Compare
e6999c4
to
be6afe7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r+wc
(Okay, so maybe you should've asked for a US reviewer)
/// Gets the currently stored value as JSON-encoded string. | ||
/// | ||
/// This doesn't clear the stored value. | ||
pub fn test_get_value(&self, ping_name: Option<String>) -> Option<String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment: It does strike me as... asymmetric that we set a JsonValue
but get a String
. I understand why get is a String
: glean-core doesn't know the structure. And I'm guessing set is a JsonValue
because it's the strongest type we can manage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, to be addressed in a followup
This allows to actually inspect the files after they have been sent.
Requires mozilla/glean_parser#587
Far from ready, requires tests and more work, e.g. APIs so we can properly implement it for C++ and JS.