-
Notifications
You must be signed in to change notification settings - Fork 474
Remove service-glean's hard-dependency on httpurlconnection #6875
Remove service-glean's hard-dependency on httpurlconnection #6875
Conversation
if (httpClient != null) { | ||
return GleanCoreConfiguration(serverEndpoint, channel, maxEvents, httpClient) | ||
} else { | ||
return GleanCoreConfiguration(serverEndpoint, channel, maxEvents) |
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.
Is our default behaving the same as lib-fetch-httpurlconnection
? Please note that this will have impact on Lockwise and Firefox Reality. I'd rather making this explicit..
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.
Make it explicit how?
We don't want to depend on lib-fetch-httpurlconnection
.
We implement a ping uploader here: https://github.com/mozilla/glean/blob/4aab82a58c0e9db1577ebd10019447155bb846e7/glean-core/android/src/main/java/mozilla/telemetry/glean/net/HttpURLConnectionUploader.kt#L20
It's defering upload to java.net.HttpURLConnection
.
IMO it makes more sense to not redefine anything like that in a-c.
That only means we would now introduce a second place we need to change if we ever touch the Glean core uploader.
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.
Sorry I wasn't clear :-) I mean let's just drop any default and force users to explicitly state what they want.
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.
If we only drop it here than that configuration API differs between a-c and direct use, which seems ... not good
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.
The API already differs between a-c and direct use: a-c depends on ConceptFetch
while direct use doesn't know about it.
If the intent here is to make sure Mozilla users are intentional with the upload mechanism, I'm afraid this is the only option.
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.
That would mean we're also breaking all the current users, as they are now forced to pass this explicitly.
Are we okay with doing that?
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.
Are we okay with doing that?
This is what @pocmo was asking: I'm not a fan of this dance, to be honest, but we're living in A-C and we should respect the A-C's guidelines :-)
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.
but we're living in A-C and we should respect the A-C's guidelines :-)
I'm open to discussing this though. :)
I think it is a fairly cheap breakage: Apps can fix this with one line of code. Fenix shouldn't break since Fenix should never use anything but Gecko for network communication. If it breaks Fenix then this would be good. :)
Lockwise: Yeah, they would probably need to add HttpUrlConnectionClient
manually once. I think that is fine.
Firefox Reality: Haven't looked at it yet. But if it breaks then this may actually be good since I assume there is a strong reason to also use Gecko for all network communication in Firefox Reality. And if they have been using HttpUrlConnection
instead then this may exactly be one of the cases where the default was not obvious (or having the choice) and where the default is the opposite of what we wanted.
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.
Firefox Reality seems to use the default and I filed an issue for changing that: MozillaReality/FirefoxReality#3339
202d52d
to
7f05b79
Compare
I updated the PR now. I still need to test that it still works in the mentioned apps and probably prepare PRs for them. |
components/service/glean/src/main/java/mozilla/components/service/glean/config/Configuration.kt
Outdated
Show resolved
Hide resolved
* This looses the lazyness of creating the `client`. | ||
*/ | ||
@JvmStatic | ||
fun fromClient(client: Client): ConceptFetchHttpUploader { |
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.
Firefox Reality would need to use a different uploader. Do we need to do this for other uploaders as well? @pocmo
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.
I'm not sure I fully understand. The 1:1 migration for Firefox Reality would be to continue using HttpUrlConnectionClient
(MozillaReality/FirefoxReality#3339)?
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.
Client
is coming from concept-fetch.
So as long as whatever they want to use adheres to that they're fine.
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. Let's update our Glean docs for A-C consumers. Also cc @mdboom
148be00
to
510fad9
Compare
Codecov Report
@@ Coverage Diff @@
## master #6875 +/- ##
============================================
+ Coverage 77.07% 77.24% +0.17%
+ Complexity 4867 4711 -156
============================================
Files 653 641 -12
Lines 24011 23290 -721
Branches 3519 3409 -110
============================================
- Hits 18506 17991 -515
+ Misses 4040 3898 -142
+ Partials 1465 1401 -64 Continue to review full report at Codecov.
|
...service/glean/src/main/java/mozilla/components/service/glean/net/ConceptFetchHttpUploader.kt
Show resolved
Hide resolved
f79abaf
to
51219e9
Compare
We still test with it, so we keep it around.⚠️ **BREAKING CHANGE**⚠️ Users will need to supply a configuration with a ping uploader to Glean now. A default lib-fetch-httpurlconnection implementation can be used like this: ```kotlin import mozilla.components.lib.fetch.httpurlconnection.HttpURLConnectionClient; import mozilla.components.service.glean.config.Configuration; import mozilla.components.service.glean.net.ConceptFetchHttpUploader; val config = Configuration(httpClient = ConceptFetchHttpUploader(lazy { HttpURLConnectionClient() })) Glean.initialize(context, true, config) ``` For Java this becomes: ```java import mozilla.components.lib.fetch.httpurlconnection.HttpURLConnectionClient; import mozilla.components.service.glean.config.Configuration; import mozilla.components.service.glean.net.ConceptFetchHttpUploader; ConceptFetchHttpUploader httpClient = ConceptFetchHttpUploader.fromClient(new HttpURLConnectionClient()); Configuration config = new Configuration(httpClient); Glean.INSTANCE.initialize(context, true, config); ``` Co-authored-by: Alessio Placitelli <alessio.placitelli@gmail.com>
51219e9
to
5bb9171
Compare
Please go ahead and merge this ;) |
bors r=Dexterp37 |
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.
👍
Build succeeded: |
Remove service-glean's hard-dependency on httpurlconnection
We still test with it, so we keep it around.
BREAKING CHANGE:
Users will need to supply a configuration with a ping uploader to Glean
now.
A default lib-fetch-httpurlconnection implementation can be used like
this:
For Java this becomes:
Fixes #6660 (at least partly)
I'm not familiar enough with service-experiments, so @travis79 would need to take a look.
cc @pocmo for visibility.
Pull Request checklist
After merge