-
Notifications
You must be signed in to change notification settings - Fork 20
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
Adds carbondb #61
Adds carbondb #61
Conversation
…the future depending on the machine.
Old Energy EstimationEco-CI Output:
📈 Energy graph:
2.00 ┼─────────────
Watts over time 🌳 CO2 Data: |
Old Energy EstimationEco-CI Output:
📈 Energy graph:
2.00 ┼─────────────
Watts over time 🌳 CO2 Data: |
source "$(dirname "$0")/vars.sh" add_var MEASUREMENT_RAN true | ||
|
||
if [ -z "$cb_machine_uuid" ]; then | ||
cb_machine_uuid=$(uuidgen) |
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.
Github runners can actually be placed on the exact same machine. I do not know how uuidgen
works under the hood but I see a fair chance for a lot of collisisone here ...?
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.
The uuidgen command generates a Universally Unique IDentifier (UUID), a 128-bit value guaranteed to be unique over both space and time.
scripts/vars.sh
Outdated
get_carbon_intensity() { | ||
latitude=$1 | ||
longitude=$2 | ||
token="your_electricity_maps_token_here" |
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.
How is this token ever gonna get filled? It is no variable?
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.
Never, as electricity maps works without a token. The string is not chose very well. And there is some legacy code there too. Added the option for a var
scripts/vars.sh
Outdated
|
||
if echo "$response" | jq '.carbonIntensity' | grep -q null; then | ||
echo "Required carbonIntensity is missing. Exiting" >&2 | ||
return 1 |
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.
you return a 1 here, but later you check for empty string. Is that intended and you want to proceed with 1 as a valida value?
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.
1 is for error. The check for empty string was from before I wrote stuff the stderr
scripts/make_measurement.sh
Outdated
shift | ||
;; | ||
|
||
\?) | ||
echo "Invalid option -$OPTARG" >&2 |
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.
Do you just want to echo here? Or maybe break?
@@ -35,6 +39,19 @@ inputs: | |||
description: 'Base URL of the Github API to send data to. Default is api.github.com, but can be changed to your hostname if you have Github Enterprise' | |||
default: 'api.github.com' | |||
required: false | |||
company-uuid: | |||
description: 'If you want to add data to the CarbonDB you can set this to your company UUID' | |||
default: '' |
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.
The other optional arguments like label
are default null. Is it possible to unify that to either?
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 used empty string here as I can just pass it into the curl post command and not really care as the backend will then ignore it. I am not sure if handling null will make it a lot cleaner
- `send-data`: (optional) (default: true) | ||
- Send metrics data to metrics.green-coding.io to create and display badge, and see an overview of the energy of your CI runs. Set to false to send no data. The data we send are: the energy value and duration of measurement; cpu model; repository name/branch/workflow_id/run_id; commit_hash; source (GitHub or GitLab). We use this data to display in our green-metrics-tool front-end here: https://metrics.green-coding.io/ci-index.html | ||
- Send metrics data to metrics.green-coding.io to create and display badge, and see an overview of the energy of your CI runs. Set to false to send no data. The data we send are: the energy value and duration of measurement; cpu model; repository name/branch/workflow_id/run_id; commit_hash; source (GitHub or GitLab). We use this data to display in our green-metrics-tool front-end here: https://metrics.green-coding.io/ci-index.html | ||
- `show-carbon`: (optional) (default: 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.
Having this default true is nice, but how will this work without the electricitymaps token?
@ArneTR feedback incorporated |
Old Energy EstimationEco-CI Output:
📈 Energy graph:
2.00 ┼─────────────
Watts over time 🌳 CO2 Data: |
Old Energy EstimationEco-CI Output:
📈 Energy graph:
2.00 ┼─────────────
Watts over time 🌳 CO2 Data: |
Old Energy EstimationEco-CI Output:
📈 Energy graph:
2.00 ┼─────────────
Watts over time 🌳 CO2 Data: |
Thanks for the clarification that the token is optional. I am not a fan however of the multiple ways to submit input. All other info is supplied via arguments. Only the Electricitymaps Token is now an exported environment var. What is the rationale behind this? In any case: I would favor for supplying it as argument and also making it a secret by default. Not a classic ENV.
Carbon Intensity for this location: 99 (g / kg / tons per Wh, J , kwH ?) |
The problem is that when we do it with a secret/ not as env we will need to add it as a parameter to the |
Old Energy EstimationEco-CI Output:
📈 Energy graph:
1.81 ┤ ╭╮
1.81 ┤ ││
1.81 ┤ ││
1.80 ┤ ││
1.80 ┤ ││
1.79 ┤ ││
1.79 ┤ ││
1.78 ┤ ││
1.78 ┤ ││
1.77 ┤ ││
1.77 ┼───────────╯╰
Watts over time 🌳 CO2 Data: |
Old Energy EstimationEco-CI Output:
📈 Energy graph:
2.00 ┼─────────────
Watts over time 🌳 CO2 Data: |
TBD tomorrow |
Old Energy EstimationEco-CI Output:
📈 Energy graph:
2.00 ┼─────────────
Watts over time 🌳 CO2 Data: |
Old Energy EstimationEco-CI Output:
📈 Energy graph:
2.00 ┼─────────────
Watts over time ❌ CO2 Data: |
Old Energy EstimationEco-CI Output:
📈 Energy graph:
1.90 ┤ ╭
1.89 ┤ │
1.87 ┤ │
1.86 ┤ │
1.85 ┤ │
1.83 ┤ │
1.82 ┤ │
1.81 ┤ │
1.80 ┤ │
1.78 ┤ │
1.77 ┼────────────╯
Watts over time 🌳 CO2 Data: |
Old Energy EstimationEco-CI Output:
📈 Energy graph:
2.00 ┼─────────────
Watts over time ❌ CO2 Data: |
Eco-CI Output:
📈 Energy graph:
3.72 ┤ ╭╮
3.53 ┤ ││
3.33 ┤ ││
3.14 ┤ ││
2.94 ┤ ││
2.74 ┤ ││
2.55 ┤ ││
2.35 ┤ ││
2.16 ┤ ││
1.96 ┤ ││
1.77 ┼─────╯╰──────
Watts over time 🌳 CO2 Data: |
@ArneTR ready to merge
|
No description provided.