chore(repo): Expand CW Metric logging to all tests #3912

fjnoyp · 2023-10-07T12:34:34Z

Issue #, if available:

Description of changes:

Add logic to send Cloudwatch metrics from all Github Action workflows.

Update aft to generate workflows that track metrics.
Update base github workflows to track metrics.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

fjnoyp · 2023-10-07T18:46:23Z

Please note that all dart unit tests on beta have been failing since an unknown time due to a compile issue with Object on a smithy http_operation. This will need to be fixed separately.

Failing scheduled dart test (same for all category dart tests):

https://github.com/aws-amplify/amplify-flutter/actions/runs/6439416299/job/17487065066

1. Take working-directory instead of package-name to find category 2. Matrix is not required in cases when matrix does not exist 3. Remove category input, will be calculated using working-directory input instead. 4. Refactory Github API response parse logic to use generated class files

Generated workflows now need additional permissions and inherit secrets to enable their sub workflow calls to be authorized to log metrics.

This reverts commit b19c40b.

fjnoyp · 2023-10-09T16:20:03Z

.github/composite_actions/log_cw_metric/action.yaml

@@ -8,12 +8,11 @@ inputs:
  github-token:
    required: true
    description: Github token for requesting failing steps.
-  package-name:
-    description: The name of the package being tested
+  working-directory:


Switch parameter sent because package-name does not give sufficient information on category information for smithy and other minor categories.

fjnoyp · 2023-10-09T16:20:21Z

.github/composite_actions/log_cw_metric/action.yaml

@@ -22,9 +21,6 @@ inputs:
  test-type:
    description: canary, integration, unit testType.
    required: true
-  category:


This is now computed based on working-directory

fjnoyp · 2023-10-09T16:20:56Z

.github/composite_actions/log_cw_metric_wrapper/action.yaml

-        package-name: ${{ inputs.package-name }}
-        matrix: ${{ toJson(matrix) }}
+        working-directory: ${{ inputs.working-directory }}
+        matrix: ${{ toJson(matrix) != 'null' && toJson(matrix) || '' }}


Updated to handle null matrix cases

fjnoyp · 2023-10-09T16:21:16Z

.github/workflows/amplify_analytics_pinpoint_android.yaml

+# These permissions are needed to interact with GitHub's OIDC Token endpoint.
+permissions:
+  id-token: write
+  contents: read


All workflows need these permissions now in order to send the Cloudwatch Metric

fjnoyp · 2023-10-09T16:23:23Z

.github/workflows/dart_ddc.yaml

@@ -12,8 +12,7 @@ on:
        type: string

 jobs:
-  test:
-    name: Test
+  test-dart-ddc:


NOTE: in order to properly distinguish jobs within a given workflow run, the jobs must not have a name field set and must have a unique identifiable name. Please see the comments added to log_cw_metric.dart for more information.

Jordan-Nelson · 2023-10-09T16:16:03Z

.github/composite_actions/log_cw_metric_wrapper/action.yaml

-        package-name: ${{ inputs.package-name }}
-        matrix: ${{ toJson(matrix) }}
+        working-directory: ${{ inputs.working-directory }}
+        matrix: ${{ toJson(matrix) != 'null' && toJson(matrix) || '' }}


I'm not following this. Is there a way to write this more clearly in code? If not, can you add a comment.

Good point, this code can be improved. Updated to:

# Sending a null value will cause the action to fail so we must send an empty string here. matrix: ${{ matrix == '' && '{}' || toJSON(matrix) }}

If the matrix value is empty we send a blank {} else we convert to json and send back.

Jordan-Nelson · 2023-10-09T16:16:44Z

.github/workflows/amplify_analytics_pinpoint.yaml

-permissions: read-all
+# These permissions are needed to interact with GitHub's OIDC Token endpoint.
+permissions:
+  id-token: write


Why is write access needed?

Setting the permissions to write is required in order to request an OpenID Connect JWT.

https://stackoverflow.com/questions/72183048/what-is-the-permission-scope-of-id-token-in-github-action

Removing this permission causes logging to fail to assume the token.

We need a token in order to post the log to cloudwatch, I assume?

Yes that's correct.

This utilizes the same framework that AppSec signed off several months ago when we just started sending Cloudwatch logs from our canaries.

actions/bin/log_cw_metric.dart

haverchuck · 2023-10-09T21:48:32Z

.github/composite_actions/log_cw_metric_wrapper/action.yaml

+        working-directory: ${{ inputs.working-directory }}
+
+        # Sending a null value will cause the action to fail so we must send an empty string here.
+        matrix: ${{ matrix == '' && '{}' || toJSON(matrix) }}


Question asked out of ignorance: Where does the matrix var come from, and why wouldn't we do ${{ toJSON(matrix) || matrix == '' && '{}' }}?

matrix is a github context variable see: https://docs.github.com/en/actions/learn-github-actions/contexts#matrix-context

I guess we could do it that way as it's just an inversion of the logic.

I read this statement as:

if matrix equals "{}", assign the matrix yaml value to "true".
ELSE
parse the matrix yaml value from json

I don't understand that logic. Why would we set the matrix yaml value to "true"? Or am I misreading this? I'm very new to GH actions so it's entirely possible I am missing something crucial here.

Also - I don't understand how a single value can evaluate to both '' and '{}' at the same time? Is this javascript?

We don't have ternary operators in Github Actions but that's essentially what we're trying to do here. If matrix is null we want to send {} otherwise we send a json version of matrix.

To do that we first check if matrix environment variable is empty or not. If it's empty we send {} this is achieved via &&, otherwise (||) we send toJSON(matrix).

Yes it is a javascript style logical operators. It is not evaluating to both '' and '{}' at the same time, it is using the '' to check matrix value before returning {} in that case.

${{ matrix == '' && '{}' || toJSON(matrix) }}

* chore(repo): Update Metric Logging Logic 1. Take working-directory instead of package-name to find category 2. Matrix is not required in cases when matrix does not exist 3. Remove category input, will be calculated using working-directory input instead. 4. Refactory Github API response parse logic to use generated class files Generated workflows now need additional permissions and inherit secrets to enable their sub workflow calls to be authorized to log metrics.

fjnoyp requested a review from a team as a code owner October 7, 2023 12:34

fjnoyp self-assigned this Oct 7, 2023

fjnoyp changed the title ~~Feat/mine/finalized dart action v4~~ chore(repo): Expand CW Metric logging to all tests Oct 7, 2023

kyle added 8 commits October 9, 2023 09:18

chore(repo): Update common workflow files to log CW

5f63e70

chore(repo): run updated aft generate with new permissions

98432de

Generated workflows now need additional permissions and inherit secrets to enable their sub workflow calls to be authorized to log metrics.

TEMP: disable schedule log block

b19c40b

fix aft generate command

28435df

fix: use proper canary runners

fbce880

fix(repo): add missing pub_server category

ef38ff6

Revert "TEMP: disable schedule log block"

3c01a85

This reverts commit b19c40b.

fjnoyp force-pushed the feat/mine/finalizedDartActionV4 branch from 451aa57 to 3c01a85 Compare October 9, 2023 16:19

fjnoyp commented Oct 9, 2023

View reviewed changes

add missing description update

03a8358

fjnoyp commented Oct 9, 2023

View reviewed changes

minor typo fix

0055bf5

Jordan-Nelson reviewed Oct 9, 2023

View reviewed changes

kyle added 2 commits October 9, 2023 11:03

improve matrix input

ff8fc13

PR comments

4d6e399

fjnoyp requested a review from Jordan-Nelson October 9, 2023 20:16

Jordan-Nelson previously approved these changes Oct 9, 2023

View reviewed changes

haverchuck reviewed Oct 9, 2023

View reviewed changes

haverchuck previously approved these changes Oct 9, 2023

View reviewed changes

fjnoyp dismissed stale reviews from haverchuck and Jordan-Nelson via 4d6e399 October 9, 2023 23:17

fjnoyp force-pushed the feat/mine/finalizedDartActionV4 branch from 4d54849 to 4d6e399 Compare October 9, 2023 23:17

fjnoyp requested review from haverchuck and Jordan-Nelson October 10, 2023 13:20

Jordan-Nelson approved these changes Oct 10, 2023

View reviewed changes

haverchuck approved these changes Oct 10, 2023

View reviewed changes

fjnoyp merged commit 40f20fc into main Oct 10, 2023
290 of 335 checks passed

fjnoyp deleted the feat/mine/finalizedDartActionV4 branch October 10, 2023 16:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(repo): Expand CW Metric logging to all tests #3912

chore(repo): Expand CW Metric logging to all tests #3912

fjnoyp commented Oct 7, 2023

fjnoyp commented Oct 7, 2023

fjnoyp Oct 9, 2023

fjnoyp Oct 9, 2023

fjnoyp Oct 9, 2023

fjnoyp Oct 9, 2023

fjnoyp Oct 9, 2023

Jordan-Nelson Oct 9, 2023

fjnoyp Oct 9, 2023

Jordan-Nelson Oct 9, 2023

fjnoyp Oct 9, 2023

haverchuck Oct 9, 2023

fjnoyp Oct 9, 2023

fjnoyp Oct 9, 2023

haverchuck Oct 9, 2023

fjnoyp Oct 9, 2023

haverchuck Oct 9, 2023 •

edited

Loading

fjnoyp Oct 9, 2023

chore(repo): Expand CW Metric logging to all tests #3912

chore(repo): Expand CW Metric logging to all tests #3912

Conversation

fjnoyp commented Oct 7, 2023

fjnoyp commented Oct 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haverchuck Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haverchuck Oct 9, 2023 •

edited

Loading