🚶‍➡️ [DNM] feat: new controller_build_info metric #13269

Joibel · 2024-06-28T14:17:21Z

One of the new metrics of #12589.

Add new, static metric which contains labels from the product version fields.

This is common good practice, and puts the information about which version of a controller was running that a given point in time.

This can be useful when diagnosing issues that occurred, and also shows the progress of a version rollout when that happens.

Note to reviewers: this is part of a stack of reviews for metrics changes. Please don't merge until the rest of the stack is also ready.

Signed-off-by: Alan Clucas alan@clucas.org

agilgur5

Docs and naming comments

agilgur5 · 2024-07-01T17:40:40Z

docs/metrics.md

@@ -201,6 +201,21 @@ Metrics for the [Four Golden Signals](https://sre.google/sre-book/monitoring-dis
    Some metric attributes may have high cardinality and are marked with ⚠️ to warn you. You may need to disable this metric or disable the attribute.
 <!-- titles should be the exact metric name for deep-linking, alphabetical ordered -->
 <!-- titles are without argo_workflows prefix -->
+#### `controller_build_info`


Suggested change

#### `controller_build_info`

#### `build_info`

isn't "controller" redundant here?

we should have more server metrics as well, but they're emitted separately by separate containers, so I would think we wouldn't need to distinguish

also wonder if build_metadata might be more explicit

Or just version to match argo version

Suggested change

#### `controller_build_info`

#### `version`

version_metadata would work too

agilgur5 · 2024-07-01T17:44:58Z

docs/metrics.md

@@ -201,6 +201,21 @@ Metrics for the [Four Golden Signals](https://sre.google/sre-book/monitoring-dis
    Some metric attributes may have high cardinality and are marked with ⚠️ to warn you. You may need to disable this metric or disable the attribute.
 <!-- titles should be the exact metric name for deep-linking, alphabetical ordered -->
 <!-- titles are without argo_workflows prefix -->
+#### `controller_build_info`
+
+The build information for this workflow controller


Suggested change

The build information for this workflow controller

Build metadata for this Controller.

simplify

agilgur5 · 2024-07-01T17:46:06Z

docs/metrics.md

+
+| attribute   | explanation                                                            |
+|-------------|------------------------------------------------------------------------|
+| `version`   | The version of argo-workflows                                          |


agilgur5 · 2024-07-01T17:54:12Z

docs/metrics.md

+| attribute   | explanation                                                            |
+|-------------|------------------------------------------------------------------------|
+| `version`   | The version of argo-workflows                                          |
+| `platform`  | Platform this is running on, as go describes it e.g. `linux/amd64` |


Suggested change

| `platform` | Platform this is running on, as go describes it e.g. `linux/amd64` |

| `platform` | The [Go platform](https://go.dev/doc/install/source#environment) compiled for. Example: `linux/amd64` |

link to the docs for GOOS + GOARCH, otherwise this feels quite unclear. I'm tempted to further explain that as a combination of "OS and architecture".

it's also what it's compiled for, not necessarily what it's running on

I could not find a better official link for this either 😕

avoid Latin phrases, per k8s style guide

agilgur5 · 2024-07-01T17:56:28Z

docs/metrics.md

+|-------------|------------------------------------------------------------------------|
+| `version`   | The version of argo-workflows                                          |
+| `platform`  | Platform this is running on, as go describes it e.g. `linux/amd64` |
+| `gover`     | Version of go that built this workflow controller                  |


Suggested change

| `gover` | Version of go that built this workflow controller |

| `goversion` | Version of Go used |

why not use the full "version"? I read "gover" as "gopher" initially (which ironically works)

simplify

half of the metrics don't say "this workflow controller" and half do. be consistent

it's redundant, so we can just be consistent in removing it

also isn't the convention to use underscores? or is it different for attribute names vs metric names?

Suggested change

| `gover` | Version of go that built this workflow controller |

| `go_version` | Version of Go used |

Underscores are explicitly mentioned as word delimiters for attribute names in the convention docs: https://opentelemetry.io/docs/specs/semconv/general/attribute-naming/

So the second suggestion should be used.

It would also be be consistent with argo version which prints GoVersion

agilgur5 · 2024-07-01T18:19:39Z

docs/metrics.md

+| `commit`    | The full git SHA1 of the code that built this workflow controller      |
+| `treestate` | Whether the git tree was `dirty` or `clean` when this was built        |
+| `tag`       | The tag on the git commit or `untagged` if it was not tagged           |


agilgur5 · 2024-07-01T18:20:35Z

workflow/metrics/build_info.go

+	const nameBuildInfo = `controller_build_info`
+	err := m.createInstrument(int64Counter,
+		nameBuildInfo,
+		"Build Information for the Argo Workflows Controller",


Suggested change

"Build Information for the Argo Workflows Controller",

"Build metadata for this Controller",

same as above

agilgur5 · 2024-07-01T18:21:59Z

workflow/metrics/labels.go

+	labelBuildGoVer        string = `goversion`
+	labelBuildDate         string = `build`
+	labelBuildCompiler     string = `compiler`
+	labelBuildGitCommit    string = `commit`
+	labelBuildGitTreeState string = `treestate`
+	labelBuildGitTag       string = `tag`


Suggested change

labelBuildGoVer string = `goversion`

labelBuildDate string = `build`

labelBuildCompiler string = `compiler`

labelBuildGitCommit string = `commit`

labelBuildGitTreeState string = `treestate`

labelBuildGitTag string = `tag`

labelBuildGoVersion string = `go_version`

labelBuildDate string = `build_date`

labelBuildCompiler string = `compiler`

labelBuildGitCommit string = `git_commit`

labelBuildGitTreeState string = `git_tree_state`

labelBuildGitTag string = `git_tag`

same as above for docs, matching argo version and splitting words with underscores per OTel convention

agilgur5 · 2024-07-01T18:27:50Z

workflow/metrics/build_info.go

+
+	version := argo.GetVersion()
+	m.addInt(ctx, nameBuildInfo, 1, instAttribs{
+		{name: labelBuildVersion, value: version.Version},


is there any way to add descriptions to attributes in OTel?

agilgur5 · 2024-07-01T18:28:30Z

workflow/metrics/build_info_test.go

+
+func TestBuildInfo(t *testing.T) {
+	_, te, err := CreateDefaultTestMetrics()
+	if assert.NoError(t, err) {


could invert the conditional here as well

One of the metrics #12589. Add new, static metric which contains labels from the product version fields. This is common good practice, and puts the information about which version of a controller was running that a given point in time. This can be useful when diagnosing issues that occurred, and also shows the progress of a version rollout when that happens. Note to reviewers: this is part of a stack of reviews for metrics changes. Please don't merge until the rest of the stack is also ready. Signed-off-by: Alan Clucas <alan@clucas.org>

Joibel force-pushed the opentelemetry-configuration branch from 0ed5767 to f4a1b61 Compare July 1, 2024 07:42

Joibel force-pushed the opentelemetry-buildinfo branch from e467bdd to fbe886c Compare July 1, 2024 07:42

Joibel added area/controller Controller issues, panics area/metrics labels Jul 1, 2024

agilgur5 changed the title ~~feat: new controller_build_info metric~~ 🚶‍➡️ [DNM] feat: new controller_build_info metric Jul 1, 2024

agilgur5 mentioned this pull request Jul 1, 2024

feat: new Prometheus metrics build_info #13077

Closed

agilgur5 self-assigned this Jul 1, 2024

agilgur5 reviewed Jul 1, 2024

View reviewed changes

Joibel force-pushed the opentelemetry-configuration branch from f4a1b61 to 3030d84 Compare July 5, 2024 10:05

Joibel force-pushed the opentelemetry-buildinfo branch from fbe886c to 9d4b210 Compare July 5, 2024 10:05

Joibel force-pushed the opentelemetry-configuration branch from 3030d84 to aab55e2 Compare July 5, 2024 10:12

Joibel force-pushed the opentelemetry-buildinfo branch from 9d4b210 to e42b80d Compare July 5, 2024 10:12

Joibel force-pushed the opentelemetry-configuration branch from aab55e2 to 4c454e5 Compare July 5, 2024 10:24

Joibel force-pushed the opentelemetry-buildinfo branch from e42b80d to b9c6b77 Compare July 5, 2024 10:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🚶‍➡️ [DNM] feat: new controller_build_info metric #13269

🚶‍➡️ [DNM] feat: new controller_build_info metric #13269

Joibel commented Jun 28, 2024 •

edited by agilgur5

Loading

agilgur5 left a comment

agilgur5 Jul 1, 2024

agilgur5 Jul 1, 2024

agilgur5 Jul 1, 2024 •

edited

Loading

agilgur5 Jul 1, 2024

agilgur5 Jul 1, 2024

agilgur5 Jul 1, 2024

agilgur5 Jul 1, 2024

agilgur5 Jul 1, 2024

agilgur5 Jul 1, 2024

agilgur5 Jul 1, 2024

agilgur5 Jul 1, 2024

agilgur5 Jul 1, 2024

agilgur5 Jul 1, 2024

agilgur5 Jul 1, 2024

	The build information for this workflow controller
	Build metadata for this Controller.

	\| `version` \| The version of argo-workflows \|
	\| `version` \| The version of Argo \|

	\| `platform` \| Platform this is running on, as go describes it e.g. `linux/amd64` \|
	\| `platform` \| The [Go platform](https://go.dev/doc/install/source#environment) compiled for. Example: `linux/amd64` \|

	\| `gover` \| Version of go that built this workflow controller \|
	\| `goversion` \| Version of Go used \|

	\| `gover` \| Version of go that built this workflow controller \|
	\| `go_version` \| Version of Go used \|

	"Build Information for the Argo Workflows Controller",
	"Build metadata for this Controller",

🚶‍➡️ [DNM] feat: new controller_build_info metric #13269

Are you sure you want to change the base?

🚶‍➡️ [DNM] feat: new controller_build_info metric #13269

Conversation

Joibel commented Jun 28, 2024 • edited by agilgur5 Loading

agilgur5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agilgur5 Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Joibel commented Jun 28, 2024 •

edited by agilgur5

Loading

agilgur5 Jul 1, 2024 •

edited

Loading