-
Notifications
You must be signed in to change notification settings - Fork 486
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
Add auto-generated connected components in documentation #5791
Conversation
619f04d
to
e459ca5
Compare
511ca26
to
678ae1e
Compare
d5dd180
to
56fe272
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.
Thank you for working on this! I really like the change. I put in some comments with suggestions on how to make it more polished.
@@ -287,7 +287,7 @@ smoke-image: | |||
# | |||
|
|||
.PHONY: generate generate-crds generate-drone generate-helm-docs generate-helm-tests generate-manifests generate-dashboards generate-protos generate-ui generate-versioned-files | |||
generate: generate-crds generate-drone generate-helm-docs generate-helm-tests generate-manifests generate-dashboards generate-protos generate-ui generate-versioned-files | |||
generate: generate-crds generate-drone generate-helm-docs generate-helm-tests generate-manifests generate-dashboards generate-protos generate-ui generate-versioned-files generate-docs |
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.
generate-versioned-files
also generates docs-related content. It might be a bit confusing to have a generate-docs
alongside it.
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.
Hmmm, I am not a fan of "versioned files", cause I'm not sure what it is by reading the name. It could be a lot of things. Do you have suggestions? I'm trying not to refactor existing code here though, as it's already a big PR.
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.
TBH I cannot think of more appropriate names... generate-docs
literally runs go generate
on the docs, and generate-versioned-files
is not docs specific. It updates a template file which is in pkg/operator
└─▪ find . -type f -name "*.t"
./docs/sources/_index.md.t
./pkg/operator/defaults.go.t
I'm ok with leaving the names as they are, but I think it might be a bit confusing since people will expect a step called "generate docs" to also update the Agent version in the docs.
|
||
## Compatible components | ||
|
||
`faro.receiver` can accept arguments from the following components: |
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.
IMO saying " can accept [...] from the following components" makes it sound like the component is data telemetry from those components. However, in this case faro.erceiver
is sending data to them.
It'd help to have a more neutral language like:
faro.receiver
can be combined with any of the following components:
Or:
faro.receiver
is compatible with any of the following components:
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.
It would also help to clarify that otelcol.Consumer
is mentioned due to output -> traces
, and that LogsReceiver
is mentioned due to output -> logs
. Not sure if it's easy to do something like this:
Compatible components
output -> logs
can be configured using a component which exports [LokiLogsReceiver
]({{< relref "../compatibility/#loki-logsreceiver-exporters" >}})
output -> traces
can be configured using a component which exports [OpenTelemetryotelcol.Consumer
]({{< relref "../compatibility/#opentelemetry-otelcolconsumer-exporters" >}})
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 know that "can accept arguments" can sound misleading if one forgets that we (sometimes...) use receivers to pass data around. But without clarification that the components can be combined via either argument or export, I find it frustrating to have to figure it out yourself. It requires extra clicks and jumps around the documentation, making the reading experience a frustrating one. So I'd prefer to be specific and not more neutral.
I like the idea of providing by name the arguments/exports that can be used to combine components, but I'd like this to keep as a future improvement, not for this PR.
|
||
<!-- START GENERATED SECTION: EXPORTERS OF Targets --> | ||
|
||
{{< collapse title="discovery" >}} |
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.
It bothers me a bit that the title in {{< collapse title="" >}}
is bigger than a ###
title. So "Targets Exporters" appears smaller than "discovery".
Is there a way to make the expandable section a bit smaller? OR at least to make its title smaller?
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.
Not something I can do here, but we can probably request the UI team to help with this in a separate PR? cc @jdbaldry perhaps? Here's an illustration of the problem:
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.
Yeah it's currently locked in at a single size. It def could use a revisit to improve the way it is styled.
following components to build your Prometheus metrics pipeline: | ||
|
||
<!-- NOTE: this title is used as an anchor in links. Do not change. --> | ||
### Prometheus `MetricsReceiver` Exporters |
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.
### Prometheus `MetricsReceiver` Exporters | |
### Exporters |
This will make the ToC on the right of the web page look much cleaner. I think we can just have subsections called "Exporters" and "Receivers" for each type.
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 would break the anchors that we link to directly. I'm aware of the TOC, but I thought that it's more important to have good anchors 🤔 We could link to the parent heading though as an alternative...
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.
It seems there is a workaround to have links to duplicate titles, but not a very reliable one.... I'm happy if you just leave it as it is. Having this as a working hyperlink does feel more important.
56fe272
to
4e172b4
Compare
Thanks @ptodev and @clayton-cornell for the feedback. I've now addressed&resolved or commented on all the threads. PTAL when you can, 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.
Thank you! There are a few things we could polish over. time, but the change looks good and I'm very happy for it to be merged.
0cb987e
to
6ea8d33
Compare
* Compatible components docs generation. * feedback * feedback * feedback, thanks * feedback
Description
This PR adds:
Generation workflow
The docs can be re-generated using
make generate-docs
.There is a CI test that compares the current docs vs. the in-memory generated docs. If differences are found, the test fails and adequate error message is printed. This can be fixed by running
make generate-docs
and the changes should be reviewed as part of PR.Examples
Loki section on "Compatible Components" page. The list of
loki
namespace components is collapsed, cause it's long.And an example of "Compatible Components" section on "loki.process" reference page:
Fixes: #4754