-
Notifications
You must be signed in to change notification settings - Fork 35
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
Enable gzip compression for Sync PATCH payloads #803
Conversation
return api.createAuthenticatedJSONRequest(url: endpoints.syncPatch, method: .PATCH, authToken: try getToken(), json: body) | ||
let headers = ["Content-Encoding": "gzip"] | ||
|
||
let body = try JSONSerialization.data(withJSONObject: json, options: []).gzipped() |
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.
As discussed on zoom, we 1) could proceed here even if gzipping fails 2) it would be good to know if it fails - it is not actionable, but information that certain volume of patches fail could be a signal itself.
…error to data provider
do { | ||
return try requestMaker.makePatchRequest(with: syncRequest, clientTimestamp: timestamp, isCompressed: true) | ||
} catch let error as GzipError { | ||
dataProvider.handleSyncError(error) |
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.
This will be emitted by dataProvider.syncErrorPublisher
and handled on the client side (e.g. in SyncBookmarksAdapter
) by firing a suitable "sync failed" pixel (e.g. syncBookmarksFailed
). This is acceptable even though sync didn't actually fail here, only the compression failed. We aren't expecting huge volume of compression errors anyway, and if they become an issue we may add a dedicated pixel.
|
||
struct SyncPayloadCompressor: SyncPayloadCompressing { | ||
func compress(_ payload: Data) throws -> Data { | ||
try payload.gzipped() |
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.
You can simulate an error here by using @testable import Gzip
on the top and throw GzipError(code: 100, msg: nil)
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.
LGTM a couple of non blocking nit to consider
* main: Enable gzip compression for Sync PATCH payloads (#803) autofill: send current language in runtime config (#808) Password manager survey update (#811) on iOS allow bookmarks in top hits (#812) Rename tests to avoid conflicts (#813) Bat.js fix (fix for installing content blocking rules multiple times) (#779) Pixels automatic naming prefixing fixed (#810)
Please review the release process for BrowserServicesKit here.
Required:
Task/Issue URL: https://app.asana.com/0/0/1206919211758354/f
iOS PR: duckduckgo/iOS#2805
macOS PR: duckduckgo/macos-browser#2723
What kind of version bump will this require?: Patch
Description:
This change adds gzip compression to all Sync data PATCH requests.
Steps to test this PR:
Internal references:
Software Engineering Expectations
Technical Design Template