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

chore(repo): Updated CW Metrics #3741

Merged
merged 1 commit into from
Oct 9, 2023
Merged

Conversation

fjnoyp
Copy link
Contributor

@fjnoyp fjnoyp commented Sep 14, 2023

Issue #, if available:

Description of changes:

Build upon existing CW Metric reporting to track additional dimensions in each metric.

Refactor CW Metric reporting to use Github Actions Dart -> JS framework.

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

@fjnoyp fjnoyp requested a review from a team as a code owner September 14, 2023 18:19
@fjnoyp fjnoyp force-pushed the feat/mine/finalizedDartAction branch from a2ddf75 to c587a2c Compare September 14, 2023 18:21
@fjnoyp fjnoyp force-pushed the feat/mine/finalizedDartAction branch from c587a2c to 85aeaf8 Compare September 14, 2023 18:25
@@ -0,0 +1,103 @@
name: Log CW Metric Wrapper
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrapper for log_cw_metric

Auto fills:
repo: ${{ github.repository }}
run-id: ${{ github.run_id }}

And calls configure AWS credentials

So that workflows that need to log metrics don't need to worry about these extra details.

job-status:
description: Used to determine if we track success or failure.
required: true
job-identifier:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

github does not return the current job id. While you can get current run id, when requesting run information you get all the jobs for that run. Thus to differentiate an individual job of that run, I needed to have this parameter for finding the particular job.

@fjnoyp fjnoyp marked this pull request as draft September 14, 2023 18:32
@fjnoyp fjnoyp marked this pull request as ready for review September 18, 2023 14:36
actions/pubspec.yaml Outdated Show resolved Hide resolved
actions/pubspec.yaml Outdated Show resolved Hide resolved
actions/bin/log_cw_metric.dart Outdated Show resolved Hide resolved
actions/bin/log_cw_metric.dart Outdated Show resolved Hide resolved
actions/bin/log_cw_metric.dart Outdated Show resolved Hide resolved
Comment on lines 14 to 23
final jobStatus = core.getRequiredInput('job-status');
final jobIdentifier = core.getRequiredInput('job-identifier');
final githubToken = core.getRequiredInput('github-token');
final repo = core.getRequiredInput('repo');
final runId = core.getRequiredInput('run-id');
Copy link
Contributor

Choose a reason for hiding this comment

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

Most of this info can be pulled from the action's context instead of needing to be passed in manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks using the github context helped.

Still needed to keep some:

  1. Job status is only in the job.context.jobStatus
  2. Switched jobIdentifier (hardcoded in our workflow calls) with dart parsing logic on a matrix input

This extra complexity is largely due to Github not providing the current job id, which means we can only get the entire job list of a workflow run. We need some form of identifier to know which of those jobs is the actual job id we are currently running in.

actions/bin/log_cw_metric.dart Outdated Show resolved Hide resolved

final value = isFailed ? '1' : '0';

if (category.contains('/')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You're already going through the hassle of passing in this value, why not pass in the category itself and compute it ahead of time as part of the generate workflows command?

Copy link
Contributor Author

@fjnoyp fjnoyp Sep 20, 2023

Choose a reason for hiding this comment

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

That makes sense, though I'd rather keep our github action workflows clean and avoid adding additional params that are only needed for my logging flows.

Alternatively we can directly accept a packageName to the log metric method instead.

@dnys1
Copy link
Contributor

dnys1 commented Sep 19, 2023

I've opened #3767 to make working with the GitHub context easier.

@fjnoyp fjnoyp force-pushed the feat/mine/finalizedDartAction branch 3 times, most recently from e8264f7 to 08dac9f Compare September 25, 2023 23:41
@fjnoyp fjnoyp requested review from dnys1 and a team September 25, 2023 23:41
@fjnoyp fjnoyp changed the title Feat/mine/finalized dart action feat(repo): Updated CW Metrics Sep 26, 2023
@fjnoyp fjnoyp changed the title feat(repo): Updated CW Metrics chore(repo): Updated CW Metrics Sep 26, 2023
@fjnoyp fjnoyp force-pushed the feat/mine/finalizedDartAction branch 2 times, most recently from 62e7111 to 6a3bb4c Compare September 26, 2023 01:18
actions/pubspec.yaml Outdated Show resolved Hide resolved
actions/lib/src/node/actions/http_request.dart Outdated Show resolved Hide resolved
actions/lib/src/node/actions/http_request.dart Outdated Show resolved Hide resolved
actions/lib/src/githubJobs/github_jobs.dart Show resolved Hide resolved
actions/bin/log_cw_metric.yaml Outdated Show resolved Hide resolved
actions/bin/log_cw_metric.yaml Outdated Show resolved Hide resolved
actions/lib/src/node/actions/http_request.dart Outdated Show resolved Hide resolved
@fjnoyp fjnoyp force-pushed the feat/mine/finalizedDartAction branch from 6197c29 to 617bf67 Compare October 3, 2023 16:56
@fjnoyp fjnoyp requested a review from dnys1 October 3, 2023 18:00
@Equartey Equartey removed the request for review from dnys1 October 6, 2023 20:18
@fjnoyp fjnoyp force-pushed the feat/mine/finalizedDartAction branch 2 times, most recently from 2e75b63 to 3cec5b1 Compare October 6, 2023 20:26
@fjnoyp fjnoyp force-pushed the feat/mine/finalizedDartAction branch from 3cec5b1 to 8e93481 Compare October 6, 2023 21:02
@fjnoyp fjnoyp merged commit 12fc814 into main Oct 9, 2023
20 checks passed
@fjnoyp fjnoyp deleted the feat/mine/finalizedDartAction branch October 9, 2023 16:13
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.

4 participants