-
Notifications
You must be signed in to change notification settings - Fork 2
feat: add new emit-metrics action #65
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
Conversation
| @@ -0,0 +1,74 @@ | |||
| name: Emit Metrics | |||
| description: Emit metrics from another workflow/action using the kat tool | |||
There was a problem hiding this comment.
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"
| BuildTime:4.532:Seconds | ||
| BuildSucceeded:1:Count | ||
| ``` | ||
| required: true |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
| - name: Emit Metrics | ||
| shell: bash | ||
| run: | |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| BuildTime:4.532:Seconds | ||
| BuildSucceeded:1:Count |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Issue #, if available:
(none)
Description of changes:
Creating the emit-metrics action to invoke
kat metrics emit ...in workflows.I've created a test-emit-metrics workflow which I'll delete before merging the PR. It shows how the action can be invoked as a step:
Example output from succeeding run:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.