Skip to content
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 74 additions & 0 deletions .github/actions/emit-metrics/action.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
name: Emit Metrics
description: Emit metrics from another workflow/action using the kat tool
Copy link
Member

Choose a reason for hiding this comment

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

nit: "from another workflow/action" is implied, could be "Emit metrics using that kat tool"

inputs:
namespace:
description: The CloudWatch namespace in which to emit metrics
required: true
dimensions:
description: |-
The dimensions to include with emitted metrics, as a collection of `name=value` pairs. Multiple dimensions are
delimited by newlines (`\n`) and whitespace is trimmed from before/after each `name=value` pair. When passing
Comment on lines +9 to +10
Copy link
Member

Choose a reason for hiding this comment

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

Can this script also support space-delimited dimensions? I don't see a reason to require delimiting with newlines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the challenge in using spaces for delimiting is that dimension names/values can include spaces too. Parsing could then become ambiguous in situations like this:

Foo=Bar Operating System=Microsoft Windows Baz=Qux

It's not possible to determine whether Foo=Bar or Foo=Bar Operating, etc.

Do you foresee problems with newline delimited dimensions/metrics?

Copy link
Member

Choose a reason for hiding this comment

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

No I think this should be fine, it just stood out to me on the initial review

multiple dimensions, make sure to use YAML's block literal syntax (`|` or `|-`).

For example:
```
dimensions: |
Artifact=foo-artifact
Platform=Kotlin/JVM
Variant=External
```
required: true
metrics:
description: |-
The metrics to emit into the given namespace and using the specified dimensions. Individual metrics are written in
the form of `<name>:<value>[:<unit>]` where:
* <name> is the metric name and may include spaces but not colons (`:`)
* <value> is the numeric value of the metric
* <unit> is the CloudWatch unit name (if omitted, `None` is assumed)

Multiple metrics are delimited by newlines (`\n`) and whitespace is trimmed from before/after each metric
specification. When passing multiple metrics, make sure to use YAML's block literal syntax (`|` or `|-`).

For example:
```
metrics: |
BuildTime:4.532:Seconds
BuildSucceeded:1:Count
```
required: true
Copy link
Member

Choose a reason for hiding this comment

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

Should dimensions be required? They are not marked as .required() in our kat tool's clikt configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh whoops. Yes, they should be required. I cannot think of any instance in which we would not contextualize our metrics with at least one dimension. I'll update the kat tool accordingly.


runs:
using: composite
steps:
- name: Emit Metrics
shell: bash
run: |
Comment on lines +46 to +48
Copy link
Member

Choose a reason for hiding this comment

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

This task should first check if the kat tool has been properly set up, and return an error if it's not available

Copy link
Member

Choose a reason for hiding this comment

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

Is there a way we can idempotently invoke the setup-kat action? Then we can invoke it everywhere we use kat to ensure it's set up correctly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked but didn't find an way to run an action (e.g., setup-kat) only when it hasn't already been run. I suppose we could modify setup-kat to be idempotent by seeing if kat is present already in the path. But setup-kat itself requires aws-actions/configure-aws-credentials to have been run as well, for which we cannot so easily control idempotency.

Let me play with some options and see if we can do better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I was able to add kat verification to the action. I could probably cheese credentials verifications too but I've filed a feature request to add idempotency to configure-aws-credentials and I think that'll be the better approach long-term.

# Trim a string and echo the result
trim() {
# args:
# $1 the string to trim

echo -n "$1" | sed -e 's/^[[:space:]]*//' -e 's/[[:space:]]*$//'
}

# Convert a newline-delimited string into a single-line string delimited by spaces with each element prefixed
format() {
# args:
# $1 the prefix for each element
# $2 the newline-delimited string

PROCESSED=""
IFS=$'\n'; ARR=($2); unset IFS
for ITEM in "${ARR[@]}"; do
PROCESSED="$PROCESSED $1 \"$(trim "$ITEM")\""
done
echo -n "$(trim "$PROCESSED")"
}

NAMESPACE="--namespace \"${{ inputs.namespace }}\""
DIMENSIONS="$(format "--dimension" "${{ inputs.dimensions }}")"
METRICS="$(format "--metric" "${{ inputs.metrics }}")"
CMD="kat metrics emit $NAMESPACE $DIMENSIONS $METRICS"

echo "Executing command \"$CMD\""
eval "$CMD"
34 changes: 34 additions & 0 deletions .github/workflows/test-emit-metrics.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
name: Test the emit-metrics action
on:
workflow_dispatch:
push:
branches:
- feat-emit-metrics-action

permissions:
id-token: write

jobs:
emit-a-metric:
runs-on: ubuntu-latest
steps:
- name: Checkout sources
uses: actions/checkout@v4
- name: Configure AWS Credentials
uses: aws-actions/configure-aws-credentials@v4
with:
role-to-assume: ${{ secrets.TEST_CI_AWS_ROLE_ARN }}
aws-region: us-west-2
- name: Set up kat tool
uses: ./.github/actions/setup-kat
- name: Emit some test metrics
uses: ./.github/actions/emit-metrics
with:
namespace: Test Namespace (ignore me)
dimensions: |
Artifact=foo-artifact
Platform=Kotlin/JVM
Variant=External
metrics: |
BuildTime:4.532:Seconds
BuildSucceeded:1:Count
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious to see how these metrics strings will be formed in our real CI, particularly the values. Maybe using environment variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think in practice, the specific metrics names/types and dimension names will be static. The metric/dimension values will likely be dynamic based on available context. Env variables are an interesting idea. Are you suggesting we could accumulate metrics and dimensions in env vars or other context during a workflow and then emit them all in a final step without explicitly re-enumerating them?

Copy link
Member

Choose a reason for hiding this comment

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

Yes the metric values will need to be dynamic, so I was imagining you would set an env var while executing the build-related portions of CI, then read them back in the subsequent emit-metrics step

Loading