Implement App size metrics actions #364

AliSoftware · 2022-05-10T22:14:34Z

This work is part of the App Metrics project [ref: paaHJt-37V-p2] and implements the fastlane actions necessary to send iOS and Android app size metrics [ref: paaHJt-3od-p2#comment-6098] to the App Metrics server.

What

Implements the actions to send iOS and Android App Size metrics to our future metrics API/system

How

Implements an AppSizeMetricsHelper class — and its unit tests — to help us build a the payload for the metrics we want to send
Implements the new ios_send_app_size_metrics action which can take an .ipa, and optionally an app-thinning.plist (auto-detected if present next to the .ipa), and generates/sends the app size metrics for it for all device variants
Implements the new android_send_app_size_metrics action which can take an .aab and generates/sends the app size metrics for it and all the .apk splits built from it using bundletool

Each of those two new actions can take an api_url which points to:

Either an http:// or https:// endpoint (typically once the Metrics server is up and running, it will be the URL to its /api/grouped-metrics endpoint described here
Or a file:// URL (in which case the api_token is not required) in order to write the payload to disk — which can be useful to either debug the payload, or store it as a CI artefact, etc.

To Test

Ensure that CI runs the unit tests and that they all pass
iOS
- Add a test lane on one of the client app's Fastfile which would build the .ipa using thinning: '<thin-for-all-variants>' and method: 'enterprise' as part of gym's export_options…
- …Then calls this action, passing it a file:// URL to write the payload to disk (since we don't yet have a Metrics system hosted on a stable URL)
- 👉 This has been done and tested via this PR in WPiOS for example
Android
- Add a test lane on one of the client app's Fastfile which would build the .aab bundle…
- …Then calls this action, passing it a file:// URL to write the payload to disk (since we don't yet have a Metrics system hosted on a stable URL)
- 👉 This has been done and tested via this PR in WPAndroid for example

What's Next

Since it was created, that PR was since followed by #365 which built on top of it to add support for Universal APKs.

If you already started to review this PR — especially the parts about the Android action — I suggest to review and land this PR first and review the changes to the Android action afterwards. If you haven't started the review of this PR yet, especially haven't started to look at the Android action and its specs, feel free to merge 365 in this PR first (to merge the code changes applied to the Android action) to review the Android action and specs code all in one go.

lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_send_app_size_metrics.rb

lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_send_app_size_metrics.rb

spec/android_send_app_size_metrics_spec.rb

spec/ios_send_app_size_metrics_spec.rb

To help build a payload for the grouped-metrics API

Since we expect it to point to the full URL, including the `/api/grouped-metrics` path, in order to be as flexible as possible

AliSoftware · 2022-05-19T19:03:10Z

Update: I've created another PR #365 built on top of this one to add support for "Universal APKs" on the android_send_app_size_metrics action.

I've made it a separate PR on top of this one rather than pushing direclty on this one, because I consider that this PR is ready to review and merged as-is, is in a good state and can work standalone; so I didn't want the work on improvements from #365 to block this one from moving forward, and also because I wasn't sure if some people already started to review this PR (as it has been on ready for review for a couple of days already) and didn't want them to have to re-review everything from start if that was the case.

mokagio

I've been trying to review this PR for a while, but never had enough time set aside for it.

To avoid this becoming stale, here's a partial review covering app_size_metrics_helper only.

I like what I'm seeing so far 😄 👏 Can't wait to see this in action.

spec/app_size_metrics_helper_spec.rb

mokagio · 2022-05-23T06:29:29Z

spec/app_size_metrics_helper_spec.rb

+
+        code = metrics_helper.send_metrics(to: api_url, api_token: token, use_gzip: false)
+
+        expect(code).to eq(201)


When I first read this, I questioned whether it was valuable to assert code is 201 when that's the value we set in stub_request.

But I came to the conclusion that it is indeed valuable: To ensure send_metrics returns the HTTP response status code it received 👍

Followup question after reading the production code this test verifies (I try to read the tests first and the production code after).

How do you feel about the fact that 201 is also the value returned by the file URI code path? Could it result in a false positive here? Or, given we rely on webmock which does a lot of extra checks for us, we can rest assured that if the file URI code path is hit instead of the API one we'll know about it? I'm leaning towards the latter 👍

How do you feel about the fact that 201 is also the value returned by the file URI code path? Could it result in a false positive here?

Well, as the code comment says on this line, the fact that the file URI code path returns 201 to mimick a typical server response — and make the call size easier and more generic to write, not having to change the "success" test in the calling Fastfile when you need to switch between real server and debugging file URL — is fully intentional here.

So in fact that's fully intentional that this spec tests that the code returns 201, even for the cases when the file URI path is being tested.

So in fact that's fully intentional that this spec tests that the code returns 201, even for the cases when the file URI path is being tested.

Totally agree that it's appropriate to ensure code is 201 when the file URI path is being tested 👍

What I'm on the fence about is the https:// behavior. This code is for the https:// URI, though, and that's really what my question was about.

The https:// path returns response.code.to_i.

The tests stubs the response to return 201, the the same value that the file:// code path would return. That's what made me wonder about the chance of a false positive, since, just by looking at code, I don't think we can which path the code took.

But "given we rely on webmock which does a lot of extra checks for us, we can rest assured that if the file URI code path is hit instead of the API one we'll know about it" because between webmock and the expect(stub).to have_been_made.once the test would fail. So, all in all, I think this is okay 👍 😄

Ah I see.

But note that this expect is there not only to ensure that the status code was 201 (which we know because we stub it)… but also (and mainly?) to check that the action forwards that http status code (that the request / stub returned) as a return value of the action itself — as opposed to returning nothing, or the JSON payload, or whatnot as a return value.

lib/fastlane/plugin/wpmreleasetoolkit/helper/app_size_metrics_helper.rb

mokagio · 2022-05-23T06:33:53Z

lib/fastlane/plugin/wpmreleasetoolkit/helper/app_size_metrics_helper.rb

+      #
+      def add_metric(name:, value:, meta: nil)
+        metric = { name: name, value: value }
+        meta = (meta || {}).compact # Remove nil values if any


Thinking out loud: How would this code feel if there was a remove_nil_values_if_any(meta) call instead?

def remove_nil_values_if_any(hash) (hash || {}).compact end

The rationale for this suggestion comes from one of my favorite bits from "Clean Code" by Uncle Bob:

The thing is, your suggestion for def remove_nil_values_if_any does more than just removing nil values if there are any (this part is just what .compact does, by definition of compact). In practice your method also defaults the meta to an empty Hash if a nil value was provided for the meta parameter.

So to match what is described/auto-documented by your method's name, the correct implementation should be:

def remove_nil_values_if_any(hash) hash.compact end

And at that point, just .compact is already term of art and self-explanatory.

Alternatively, you could keep your implementation and try to find a more explanatory method name… but that would start to become quite verbose and cumbersome:

def default_to_empty_hash_if_nil_and_remove_any_nil_values_otherwise(hash) (hash || {}).compact end

An alternative to maybe make this line of code clearer and still using term-of-art methods would be to exchange the order of the two actions (but still not worth extracting in their own method):

meta = meta&.compact || {}

Since compact is term-of-art, meta&.compact is self-explanatory (ICYDK, &. is optional-chaining in ruby, just like ?. in Swift), and given that the x || {} is very idiomatic in Ruby to provide default values (that's the equivalent of x ?? [:] in Swift), the full expression would seem self-explanatory to me overall, and probably easier to parse than the parenthesised and flipped (meta || {}).compact (where the () is probably what makes it harder to grasp?).

☝️ applied in a3660c8

lib/fastlane/plugin/wpmreleasetoolkit/helper/app_size_metrics_helper.rb

mokagio

Another partial review, this time for the ios_send_app_size_metrics action

mokagio · 2022-05-24T05:24:37Z

spec/ios_send_app_size_metrics_spec.rb

+
+describe Fastlane::Actions::IosSendAppSizeMetricsAction do
+  let(:test_data_dir) { File.join(File.dirname(__FILE__), 'test-data', 'app_size_metrics') }
+  let(:fake_ipa_size) { 1337 } # The value used in the `app-thinning.plist` and `ios-metrics-payload.json` fixtures


Is this an easter egg reference to the Leet language?

It is 😛 (I often freely use both 42 and 1337 for random values as references to H2G2 and Leet language in my tests 😝 )

mokagio · 2022-05-24T05:27:31Z

spec/ios_send_app_size_metrics_spec.rb

+      }
+
+      test_app_size_action(
+        fake_ipa_size: 123_456,


I didn't know Ruby allowed this for numbers like Swift, but in hindsight it's not surprising. Neat!

Actually rubocop even complains if I don't add the _ thousand separators 😅

mokagio · 2022-05-24T05:34:20Z

lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_send_app_size_metrics.rb

+        # Check input parameters
+        api_url = URI(params[:api_url])
+        api_token = params[:api_token]
+        if (api_token.nil? || api_token.empty?) && !api_url.is_a?(URI::File)
+          UI.user_error!('An API token is required when using an `api_url` with a scheme other than `file://`')
+        end


Have you considered making the API token an optional: false parameter?

From a consumer point of view, we don't expect users to call this action with a file:// API URL, but always and only with an https:// URL plus a token. We could ignore the token in the file:// case here, which is what the send_metrics method seems to do, too.

I'm not sure I'm following your argument here 😅

we don't expect users to call this action with a file:// API URL

Which is why it has to be optional: true, otherwise fastlane itself will user_error! all by itself if you provide a file:// URL but no api_token (and why would you provide an API token when you're using file:// URL, and what would that token be anyway… be it for debugging while we don't yet have a metrics server and thus any token to provide, or if you use this action to store the metrics as artefacts on your CI rather than sending them to a metrics server)

So, yes, the token in the file:// case is ignored here (that's exactly what those lines 11–13 are for, to ignore it and allow it to be optional provided… only if using a file URL, and be required only when using a non-file URL).
Given it has a different optionality depending on the type of api_url used, fastlane won't let us use optional: false in the ConfigItem otherwise it will complain that it's missing regardless of if it was valid to omit it (with file:// urls) or not. So the only way in custom fastlane actions to provide conditional optionality is to handle it ourselves, as ConfigItem does not provide a way for that directly.

Ok I'm re-reading your comment and I get what you meant now (I was just missing some coffee earlier… ☕ ).

But in fact, even if in 80% of cases — especially once we'll have our Metrics server up and running at a stable URL — we will use a https:// URL for api_url… there are still legitimate cases were using file:// is not the exception or the "secret" case but a genuine usage we still want to cover properly (instead of treating it as if it was an easter egg):

Debugging the payloads is an obvious use case for sure, but I can see how this could be considered a rare case and not "normal operation"

But we could also imagine to use this to handle cases when the Metrics server has to go into maintenance (e.g. if we need to shut it down for a sprint or two while figuring out something broken or fixing some security issue on it…) and we would want to instead store the metrics on disk in the artefacts during that time, to be able to "replay" those payloads later once the Metrics server goes back up

Or even on a regular basis, if we decide to run the action twice, once to upload the metrics to a server, and another time to store the payload in the CI's artifacts for "backup"

Or when we want to generate the metrics locally, e.g. to compare the sizes of the app between two branches or two tags, and we'd then want to run the lane locally on our Macs in one branch then the next and compare the results

…

So imho while it definitively won't be the majority of cases, those are still legitimate ones and as such should be considered more than just easter eggs and the optionality of api_token depending on it should be handled properly as a result 😛

So imho while it definitively won't be the majority of case

Okay, cool. If that's the expectation, then it makes sense to keep the API consistent for both, rather than optimize it only for a scenario.

👍

lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_send_app_size_metrics.rb

Co-authored-by: Gio Lodi <[email protected]>

lib/fastlane/plugin/wpmreleasetoolkit/helper/app_size_metrics_helper.rb

spec/app_size_metrics_helper_spec.rb

AliSoftware · 2022-05-24T09:42:00Z

spec/app_size_metrics_helper_spec.rb

+
+        code = metrics_helper.send_metrics(to: api_url, api_token: token, use_gzip: false)
+
+        expect(code).to eq(201)


How do you feel about the fact that 201 is also the value returned by the file URI code path? Could it result in a false positive here?

Well, as the code comment says on this line, the fact that the file URI code path returns 201 to mimick a typical server response — and make the call size easier and more generic to write, not having to change the "success" test in the calling Fastfile when you need to switch between real server and debugging file URL — is fully intentional here.

So in fact that's fully intentional that this spec tests that the code returns 201, even for the cases when the file URI path is being tested.

lib/fastlane/plugin/wpmreleasetoolkit/helper/app_size_metrics_helper.rb

AliSoftware · 2022-05-24T09:53:01Z

lib/fastlane/plugin/wpmreleasetoolkit/helper/app_size_metrics_helper.rb

+      #
+      def add_metric(name:, value:, meta: nil)
+        metric = { name: name, value: value }
+        meta = (meta || {}).compact # Remove nil values if any


The thing is, your suggestion for def remove_nil_values_if_any does more than just removing nil values if there are any (this part is just what .compact does, by definition of compact). In practice your method also defaults the meta to an empty Hash if a nil value was provided for the meta parameter.

So to match what is described/auto-documented by your method's name, the correct implementation should be:

def remove_nil_values_if_any(hash) hash.compact end

And at that point, just .compact is already term of art and self-explanatory.

Alternatively, you could keep your implementation and try to find a more explanatory method name… but that would start to become quite verbose and cumbersome:

def default_to_empty_hash_if_nil_and_remove_any_nil_values_otherwise(hash) (hash || {}).compact end

An alternative to maybe make this line of code clearer and still using term-of-art methods would be to exchange the order of the two actions (but still not worth extracting in their own method):

meta = meta&.compact || {}

Since compact is term-of-art, meta&.compact is self-explanatory (ICYDK, &. is optional-chaining in ruby, just like ?. in Swift), and given that the x || {} is very idiomatic in Ruby to provide default values (that's the equivalent of x ?? [:] in Swift), the full expression would seem self-explanatory to me overall, and probably easier to parse than the parenthesised and flipped (meta || {}).compact (where the () is probably what makes it harder to grasp?).

AliSoftware · 2022-05-24T09:53:55Z

spec/ios_send_app_size_metrics_spec.rb

+
+describe Fastlane::Actions::IosSendAppSizeMetricsAction do
+  let(:test_data_dir) { File.join(File.dirname(__FILE__), 'test-data', 'app_size_metrics') }
+  let(:fake_ipa_size) { 1337 } # The value used in the `app-thinning.plist` and `ios-metrics-payload.json` fixtures


It is 😛 (I often freely use both 42 and 1337 for random values as references to H2G2 and Leet language in my tests 😝 )

AliSoftware · 2022-05-24T10:00:48Z

lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_send_app_size_metrics.rb

+        # Check input parameters
+        api_url = URI(params[:api_url])
+        api_token = params[:api_token]
+        if (api_token.nil? || api_token.empty?) && !api_url.is_a?(URI::File)
+          UI.user_error!('An API token is required when using an `api_url` with a scheme other than `file://`')
+        end


I'm not sure I'm following your argument here 😅

we don't expect users to call this action with a file:// API URL

Which is why it has to be optional: true, otherwise fastlane itself will user_error! all by itself if you provide a file:// URL but no api_token (and why would you provide an API token when you're using file:// URL, and what would that token be anyway… be it for debugging while we don't yet have a metrics server and thus any token to provide, or if you use this action to store the metrics as artefacts on your CI rather than sending them to a metrics server)

So, yes, the token in the file:// case is ignored here (that's exactly what those lines 11–13 are for, to ignore it and allow it to be optional provided… only if using a file URL, and be required only when using a non-file URL).
Given it has a different optionality depending on the type of api_url used, fastlane won't let us use optional: false in the ConfigItem otherwise it will complain that it's missing regardless of if it was valid to omit it (with file:// urls) or not. So the only way in custom fastlane actions to provide conditional optionality is to handle it ourselves, as ConfigItem does not provide a way for that directly.

lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_send_app_size_metrics.rb

To make it more readable See also #364 (comment)

AliSoftware · 2022-05-24T19:02:12Z

@mokagio Just wanted to let you know that since your last review, in addition to addressing your feedback, I also addressed an issue we encountered on Android CI, where the apkanalyzer tool that the Android action uses… was installed in a different location on disk.

I addressed this in ad5935c, by adding a new ConfigItem to allow providing an explicit path to the apkanalyzer binary in case we ever needed to be explicit… and updating the logic which tries to deduce/find the binary in PATH and ANDROID_SDK_HOME to account for the variations of the tool path used in our CI.
So, in practice, we still don't need to provide the explicit path to apkanalyzer for our case (as with the new logic it's now able to deduce/find it on its own… even on CI), but in case that default installation path changes in future Android AMIs or Docker images, we now have a way to also be explicit for it.

AliSoftware · 2022-05-24T20:02:04Z

lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_send_app_size_metrics.rb

+            apks.each do |apk|
+              UI.message("[App Size Metrics] Computing file and download size of #{File.basename(apk)}...")
+              split_name = File.basename(apk, '.apk')
+              file_size = Action.sh(apkanalyzer_bin, 'apk', 'file-size', apk, print_command: false, print_command_output: false).chomp.to_i
+              download_size = Action.sh(apkanalyzer_bin, 'apk', 'download-size', apk, print_command: false, print_command_output: false).chomp.to_i
+              metrics_helper.add_metric(name: 'APK File Size', value: file_size, metadata: { split: split_name })
+              metrics_helper.add_metric(name: 'Download Size', value: download_size, metadata: { split: split_name })
+            end


ℹ️ Note for the reviewers, in case you were tempted to make suggestions about those in this PR:

This code has since been extracted in a separate method during App Size Metrics: add support for Android Universal APKs #365, to make the code cleaner, less "massive" and more readable

The names of the metrics (APK File Size and Download Size here, but same for all others used all over in *_send_app_size_metrics) have since been DRYed into constants in App Size Metrics: add support for Android Universal APKs #365 as well

mokagio

Posting a few more comments in case I get interrupted while looking at the Android side of things.

mokagio · 2022-05-26T06:05:53Z

lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_send_app_size_metrics.rb

+        # Check input parameters
+        api_url = URI(params[:api_url])
+        api_token = params[:api_token]
+        if (api_token.nil? || api_token.empty?) && !api_url.is_a?(URI::File)
+          UI.user_error!('An API token is required when using an `api_url` with a scheme other than `file://`')
+        end


So imho while it definitively won't be the majority of case

Okay, cool. If that's the expectation, then it makes sense to keep the API consistent for both, rather than optimize it only for a scenario.

👍

lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_send_app_size_metrics.rb

mokagio · 2022-05-26T06:17:33Z

spec/app_size_metrics_helper_spec.rb

+
+        code = metrics_helper.send_metrics(to: api_url, api_token: token, use_gzip: false)
+
+        expect(code).to eq(201)


So in fact that's fully intentional that this spec tests that the code returns 201, even for the cases when the file URI path is being tested.

Totally agree that it's appropriate to ensure code is 201 when the file URI path is being tested 👍

What I'm on the fence about is the https:// behavior. This code is for the https:// URI, though, and that's really what my question was about.

The https:// path returns response.code.to_i.

The tests stubs the response to return 201, the the same value that the file:// code path would return. That's what made me wonder about the chance of a false positive, since, just by looking at code, I don't think we can which path the code took.

But "given we rely on webmock which does a lot of extra checks for us, we can rest assured that if the file URI code path is hit instead of the API one we'll know about it" because between webmock and the expect(stub).to have_been_made.once the test would fail. So, all in all, I think this is okay 👍 😄

mokagio

mokagio · 2022-05-26T06:27:11Z

spec/android_send_app_size_metrics_spec.rb

+      # Compare the payloads as pretty-formatted JSON, to make the diff in test failures more readable if one happen
+      expect(JSON.pretty_generate(JSON.parse(generated_payload))).to eq(JSON.pretty_generate(expected_payload)), 'Decompressed JSON payload was not as expected'
+      # Compare the payloads as raw uncompressed data as a final check
+      expect(generated_payload).to eq(expected_payload.to_json)


These two "triangulating" assertions (inappropriate because they're only two assertions, so not a triangle, but that's a term sometimes used for this pattern) are neat 😎

# Conflicts: # CHANGELOG.md # Gemfile.lock

AliSoftware self-assigned this May 10, 2022

AliSoftware force-pushed the app-size-metrics branch from a87c45c to 8fabca4 Compare May 12, 2022 21:25