chore: Add metrics to update-release-branch workflow #1543
Conversation
ianbotsf
added
the
no-changelog
ianbotsf
changed the title
|
A new generated diff is ready to view.
|
| duration=$(( now - ${{ steps.start.outputs.timestamp }} )) | ||
| printf 'duration=$duration\n' >> "$GITHUB_OUTPUT" | ||
| - name: Emit metrics | ||
| if: always() |
There was a problem hiding this comment.
question: What does this if do, is it necessary?
There was a problem hiding this comment.
I think it ensures we always run this step regardless of failure or cancelation
There was a problem hiding this comment.
It forces this step to always run, even if previous steps failed. By default, every step implicitly has if: success() (meaning only run this step if no previous steps failed) unless explicitly overridden.
| - name: Set start timestamp | ||
| id: start | ||
| run: printf 'timestamp=%(%s)T\n' >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
Is duration a meaningful metric for this job? Since it runs on an automated schedule at 3AM, I don't think we care if it takes 5, 15, even 30 minutes, just that it succeeds or fails.
GitHub already has a default timeout of 6 hours, so we could never get into a situation where the job runs for 24+ hours and conflicts with subsequent merge jobs.
There was a problem hiding this comment.
I believe durations are always useful, particularly when they start being larger/smaller than one expects. You're right, we don't necessarily care whether it's 5 minutes or 30 minutes but we probably care if it's always been 5 minutes and suddenly becomes 30 minutes.
Additionally, we do run this job manually from time to time which means we'd be waiting for it to complete. In that case, I believe we would care more about time.
| if: always() | ||
| uses: awslabs/aws-kotlin-repo-tools/.github/actions/emit-metrics | ||
| with: | ||
| namespace: CI metrics |
There was a problem hiding this comment.
naming: Is this name what appears in our CloudWatch dashboard, should it be "CI Metrics"?
There was a problem hiding this comment.
It is and it should be.
| metrics: | | ||
| ReleaseMergeAttempted:1:Count | ||
| ReleaseMergeSucceeded:${{ job.status == 'success' && '1' || '0' }}:Count | ||
| ReleaseMergeCancelled:${{ job.status == 'cancelled' && '1' || '0' }}:Count |
There was a problem hiding this comment.
naming suggestion: ReleaseMergeCanceled
This comment has been minimized.
This comment has been minimized.
| duration=$(( now - ${{ steps.start.outputs.timestamp }} )) | ||
| printf 'duration=$duration\n' >> "$GITHUB_OUTPUT" | ||
| - name: Emit metrics | ||
| if: always() |
There was a problem hiding this comment.
I think it ensures we always run this step regardless of failure or cancelation
| Trigger=${{ github.event_name == 'schedule' && 'schedule' || 'manual' }} | ||
| metrics: | | ||
| ReleaseMergeAttempted:1:Count | ||
| ReleaseMergeSucceeded:${{ job.status == 'success' && '1' || '0' }}:Count |
There was a problem hiding this comment.
Question: If the job is canceled or fails before this step runs, would we still be able to emit metrics?
There was a problem hiding this comment.
As you noted above, this step will always run regardless of whether previous steps have failed or the job's been canceled.
| duration=$(( now - ${{ steps.start.outputs.timestamp }} )) | ||
| printf 'duration=$duration\n' >> "$GITHUB_OUTPUT" | ||
| - name: Emit metrics | ||
| if: always() |
There was a problem hiding this comment.
Suggestion: Adding a comment explaining why this is here
|
|
A new generated diff is ready to view.
|
Affected ArtifactsChanged in size
|
3 participants
Issue #
(none)
Description of changes
This change adds the emit-metrics action to our update-release-branch workflow and tracks several new data about our release job.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.