-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
refactor: autogenerate metrics docs #13943
base: main
Are you sure you want to change the base?
Conversation
f5c28f4
to
acdca3d
Compare
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.
nits commented.
I'm slightly surprised that the new markdown tables are passing CI. I thought there was a table linter? They aren't nice and pretty like they were before.
Makefile
Outdated
@@ -632,8 +632,21 @@ clean: | |||
go clean | |||
rm -Rf test-results node_modules vendor v2 v3 argoexec-linux-amd64 dist/* ui/dist | |||
|
|||
# swagger | |||
# Built telemetry files |
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.
# Built telemetry files | |
# Build telemetry files |
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.
Fixed
docs/metrics.md
Outdated
| `phase` | The phase that the workflow has entered | | ||
| attribute | explanation | | ||
|-----------|-------------| | ||
| `phase` | The phase that the workflow has entered | | ||
| `namespace` | The namespace in which the workflow is running | |
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.
... was 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.
Chosen a past and present variant
docs/metrics.md
Outdated
| attribute | explanation | | ||
|-----------|-------------| | ||
| `feature` | The name of the feature used | | ||
| `namespace` | The namespace in which the workflow is running | |
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.
was 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.
Chosen a past and present variant
docs/metrics.md
Outdated
|
||
#### `pods_total_count` | ||
|
||
A gauge of the number of pods which have entered each phase and then observed by the controller. | ||
This is not directly controlled by the workflow controller, so it is possible for some pod phases to be missed. | ||
Total number of Pods that have entered each phase. |
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.
we doing uppercase P for pods or not?
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, no upper case Ps for you
They aren't. The linter doesn't mind it, and some have been modified to look like this elsewhere by hand. To make them pretty would need a prettifier, and this way is also easier for hand editing a bit like one sentence per line is. Not remedying. |
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 is fine, I think I'd personally prefer if we used a markdown AST like https://github.com/yuin/goldmark.
This makes it possible to render the markdown nicely, it also makes parsing and doing inline modifications much easier.
No strong preferences though, this is not critical code I guess, as long as its generating valid markdown, its fine.
Goldmark doesn't do nice rendering, it has an emphasis on reading markdown. I've implemented nao1215/markdown which brings in more dependencies, but does also make the tables prettier so that @tico24 can pretend he's happy too. |
64191c3
to
87aecd4
Compare
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 is great. Thanks!
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.
Thanks for the changes.
Signed-off-by: Alan Clucas <[email protected]>
Signed-off-by: Alan Clucas <[email protected]>
Signed-off-by: Alan Clucas <[email protected]>
87aecd4
to
83d132c
Compare
Motivation
As mentioned at some point during the opentelemetry metrics changes, it would be nice if documentation could be autogenerated.
This PR does this so that there is a single source of truth for metrics and attributes.
This makes attribute documentation consistent, and the docs match the implementation in more places.
It will help when we do something about server metrics, and also with tracing (as tracing will want managed attributes and auto-generated docs).
Modifications
Create a new yaml file
util/telemetry/builder/values.yaml
which contains the attributes and metrics.util/telemetry/builder/builder.go
converts this into go structs and documentation which is inserted into the metrics.md docs. The go structs implement a new interface for builtin metrics.This code change has the downside of making the shared utils directory aware of every metric again.
This fixes a few things:
HELP
for prometheus metrics will now exactly match the first line in the documentation.workers_busy_count
has the correct attribute name (this is a legacy metric - the implementation matched 3.5 which is correct, but the docs did not match the implementation of having a different attribute name from the other queue metrics).This can be further improved by enforcing the attributes which may mismatch at the moment, and removing all of the calls to
Name()
by instead allowing the operators to operate on the builtin metrics. This is left for a separate PR. This will also optimise the calls to the operators by pre-creating the attributes.Verification
Unit and integration tests.
Some inspection of prometheus metrics from before and after.