Skip to content
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 1650787 - Implement the JWE metric type on the core and FFI #1062

Merged
merged 4 commits into from Jul 20, 2020

Conversation

brizental
Copy link
Member

@brizental brizental commented Jul 16, 2020

I decided to separate this into two parts, this one just with the core and ffi changes, the next with the addition of this metric type to the bindings.

I found a bug on https://github.com/mozilla/glean/blob/main/glean-core/src/error_recording.rs#L61, which the bindings PR will depend on too. If you notice, we jump on the count from 2 directly to 4. I'll send a separate PR to fix that.

This implementation was based on this document.

@auto-assign auto-assign bot requested a review from travis79 Jul 16, 2020
@brizental brizental requested a review from Dexterp37 Jul 16, 2020
Copy link
Collaborator

@Dexterp37 Dexterp37 left a comment

Looks good overall. A few minor changes requested.

In hindsight, I would have preferred to have at least one language binding implemented here, to have better confidence on the "language to core" mechanism. But, for this one, it should be fine. Please note that you'll need to add docs in the language bindings PRs as well.

glean-core/tests/jwe.rs Outdated Show resolved Hide resolved
glean-core/tests/jwe.rs Outdated Show resolved Hide resolved
glean-core/tests/jwe.rs Outdated Show resolved Hide resolved
glean-core/src/metrics/jwe.rs Outdated Show resolved Hide resolved
glean-core/src/metrics/jwe.rs Outdated Show resolved Hide resolved
glean-core/src/metrics/jwe.rs Outdated Show resolved Hide resolved
@brizental
Copy link
Member Author

brizental commented Jul 17, 2020

In hindsight, I would have preferred to have at least one language binding implemented here, to have better confidence on the "language to core" mechanism.

I have kotlin, swift and c# implemented already. I can add any one of them to this PR if you prefer.

@Dexterp37
Copy link
Collaborator

Dexterp37 commented Jul 17, 2020

In hindsight, I would have preferred to have at least one language binding implemented here, to have better confidence on the "language to core" mechanism.

I have kotlin, swift and c# implemented already. I can add any one of them to this PR if you prefer.

Nah, that's fine, let's not over-rotate on this for now. Let's fix the problems here and move on. Next time we can do it differently! :)

@brizental brizental force-pushed the 1650787-jwe branch 2 times, most recently from 069b059 to afd1408 Compare Jul 17, 2020
glean-core/src/lib_unit_tests.rs Outdated Show resolved Hide resolved
@brizental brizental requested a review from Dexterp37 Jul 17, 2020
Copy link
Collaborator

@Dexterp37 Dexterp37 left a comment

r+wc!

glean-core/src/metrics/jwe.rs Outdated Show resolved Hide resolved
brizental added 4 commits Jul 20, 2020
* Fix RFC link in `validate_base64url_encoding`;
* Validate each field in JWE separately and specifically;
* Fix comment for `set` function;
* Add tests for get_test_value_* functions;
* Apply lints from new Rust version.
@brizental brizental merged commit 0d7532f into mozilla:main Jul 20, 2020
32 of 33 checks passed
@brizental brizental deleted the 1650787-jwe branch Jul 20, 2020
@brizental brizental restored the 1650787-jwe branch Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants