Skip to content
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

Merged
merged 5 commits into from
Dec 4, 2023

Conversation

thampiotr
Copy link
Contributor

@thampiotr thampiotr commented Nov 16, 2023

Description

This PR adds:

  • An auto-generated section "Compatible Components" on each component's reference page that lists the most common ways the components can be connected. The "most common ways" is determined by type of data being used: Targets, Prom metrics, Loki logs, Pyroscope profiles, OTEL.
  • A new "Compatible Components" page which lists some of the most common data types used in telemetry pipelines and which components can be used to either export or consume these data types.
  • Code required to generate these pages.
  • Tests that verify that the checked-in docs match with in-memory generated docs, so we can verify that they are being updated correctly. This test also allows to overwrite local files when developer wants to update the docs locally with the generated versions.

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.
image

And an example of "Compatible Components" section on "loki.process" reference page:
image

Fixes: #4754

@thampiotr thampiotr force-pushed the thampiotr/component-x-links branch 2 times, most recently from 619f04d to e459ca5 Compare November 27, 2023 12:00
@thampiotr thampiotr mentioned this pull request Nov 27, 2023
8 tasks
@thampiotr thampiotr changed the title [DRAFT] connected components Add auto-generated connected components in documentation Nov 28, 2023
@thampiotr thampiotr marked this pull request as ready for review November 28, 2023 13:29
Copy link
Contributor

@ptodev ptodev left a 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
Copy link
Contributor

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.

Copy link
Contributor Author

@thampiotr thampiotr Nov 28, 2023

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.

Copy link
Contributor

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.

component/pyroscope/ebpf/ebpf_placeholder.go Outdated Show resolved Hide resolved

## Compatible components

`faro.receiver` can accept arguments from the following components:
Copy link
Contributor

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:

Copy link
Contributor

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 [Loki LogsReceiver]({{< relref "../compatibility/#loki-logsreceiver-exporters" >}})

output -> traces can be configured using a component which exports [OpenTelemetry otelcol.Consumer]({{< relref "../compatibility/#opentelemetry-otelcolconsumer-exporters" >}})

Copy link
Contributor Author

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" >}}
Copy link
Contributor

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?

Copy link
Contributor Author

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:
image

Copy link
Contributor

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.

docs/sources/flow/reference/compatibility/_index.md Outdated Show resolved Hide resolved
docs/sources/flow/reference/compatibility/_index.md Outdated Show resolved Hide resolved
following components to build your Prometheus metrics pipeline:

<!-- NOTE: this title is used as an anchor in links. Do not change. -->
### Prometheus `MetricsReceiver` Exporters
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
### 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.

Copy link
Contributor Author

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...

Copy link
Contributor

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.

docs/generator/links_to_types.go Outdated Show resolved Hide resolved
docs/sources/flow/reference/compatibility/_index.md Outdated Show resolved Hide resolved
docs/sources/flow/reference/compatibility/_index.md Outdated Show resolved Hide resolved
docs/sources/flow/reference/compatibility/_index.md Outdated Show resolved Hide resolved
docs/sources/flow/reference/compatibility/_index.md Outdated Show resolved Hide resolved
docs/sources/flow/reference/compatibility/_index.md Outdated Show resolved Hide resolved
docs/sources/flow/reference/compatibility/_index.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
@thampiotr
Copy link
Contributor Author

Thanks @ptodev and @clayton-cornell for the feedback. I've now addressed&resolved or commented on all the threads. PTAL when you can, thanks.

Copy link
Contributor

@ptodev ptodev left a 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.

@thampiotr thampiotr merged commit 2a7bb31 into main Dec 4, 2023
10 checks passed
@thampiotr thampiotr deleted the thampiotr/component-x-links branch December 4, 2023 16:20
@erikbaranowski erikbaranowski mentioned this pull request Dec 7, 2023
4 tasks
BarunKGP pushed a commit to BarunKGP/grafana-agent that referenced this pull request Feb 20, 2024
* Compatible components docs generation.

* feedback

* feedback

* feedback, thanks

* feedback
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 21, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make designing pipelines easier by providing lists of compatible components
3 participants