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 1597761 - Don't create PingRequest if body exceeds 1MB #1098

Merged
merged 6 commits into from Jul 29, 2020

Conversation

brizental
Copy link
Member

@brizental brizental commented Jul 27, 2020

This required a bit more changes than I expected because of the need to record metrics.

Data review for the metrics recorded here is still ongoing.

@brizental brizental requested a review from badboy Jul 27, 2020
Copy link
Member

@badboy badboy left a comment

Good start.
See inline comments, mostly about being explicit about the units we use.

We should probably also document that we discard pings bigger than 1 Mb (and that this applies to after compression).
@Dexterp37 What would be a good place to document our guarantees around pings? Do we already have something (e.g. for the rate limiting)?

glean-core/metrics.yaml Outdated Show resolved Hide resolved
glean-core/src/upload/request.rs Outdated Show resolved Hide resolved
glean-core/src/upload/request.rs Show resolved Hide resolved
glean-core/src/upload/request.rs Outdated Show resolved Hide resolved
glean-core/src/upload/request.rs Outdated Show resolved Hide resolved
glean-core/src/upload/mod.rs Outdated Show resolved Hide resolved
glean-core/src/upload/mod.rs Outdated Show resolved Hide resolved
glean-core/src/upload/mod.rs Outdated Show resolved Hide resolved
glean-core/src/upload/mod.rs Outdated Show resolved Hide resolved
@brizental
Copy link
Member Author

brizental commented Jul 28, 2020

Do we already have something (e.g. for the rate limiting)?

For the rate limiting we have a note on the internal documentation: https://mozilla.github.io/glean/book/dev/core/internal/upload.html?highlight=rate,limiting#1 and another note on the "Pings" page: https://mozilla.github.io/glean/book/user/pings/index.html?highlight=rate#ping-submission.

We could maybe create a "Limitations" section on the "Pings" page, under the "Ping submission" section to list both in a more structured manner.

@brizental brizental requested a review from badboy Jul 28, 2020
@Dexterp37
Copy link
Contributor

Dexterp37 commented Jul 28, 2020

Do we already have something (e.g. for the rate limiting)?

For the rate limiting we have a note on the internal documentation: https://mozilla.github.io/glean/book/dev/core/internal/upload.html?highlight=rate,limiting#1 and another note on the "Pings" page: https://mozilla.github.io/glean/book/user/pings/index.html?highlight=rate#ping-submission.

We could maybe create a "Limitations" section on the "Pings" page, under the "Ping submission" section to list both in a more structured manner.

@badboy @brizental I think the above is a good idea!

brizental added 5 commits Jul 28, 2020
* Use usize instead of u64 in PingBodyOverflow error;
* Be explicit about bytes unit in ping body size comments;
* Consume pending_pings array in correct order;
* Use meaningful value for body_max_size request tests;
Also...
* Correct data-review link for discarded_exceeding_pings_size
* Add changelog entry about ping tagging for iOS (forgot on previous PR)
badboy
badboy approved these changes Jul 29, 2020
* General
* Limit ping request body size to 1MB. ([#1098](https://github.com/mozilla/glean/pull/1098))
* iOS
* Implement ping tagging (i.e. the `X-Source-Tags` header) through custom URL ([#1100](https://github.com/mozilla/glean/pull/1100)).
Copy link
Member

@badboy badboy Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:D was that missed in that PR? Good to still get it in.

Copy link
Member Author

@brizental brizental Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeap, I am trying to remind myself everytime, but still miss it sometimes.

@brizental brizental merged commit 218ff33 into mozilla:main Jul 29, 2020
32 of 33 checks passed
@brizental brizental deleted the 1597761-one-mb-pings branch Jul 29, 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

4 participants