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

[docs/collector/building] add more context for what you can build #4343

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

hughesjj
Copy link

As per discussion 3986, add some more context over what you may build in the collector (intends to cover #1 and #2 under building)

@hughesjj hughesjj requested review from a team as code owners April 23, 2024 05:33
@hughesjj hughesjj requested review from codeboten and removed request for a team April 23, 2024 05:33
content/en/docs/collector/building/_index.md Outdated Show resolved Hide resolved
content/en/docs/collector/building/_index.md Outdated Show resolved Hide resolved
content/en/docs/collector/building/_index.md Outdated Show resolved Hide resolved
content/en/docs/collector/building/_index.md Outdated Show resolved Hide resolved
content/en/docs/collector/building/_index.md Outdated Show resolved Hide resolved
content/en/docs/collector/building/_index.md Outdated Show resolved Hide resolved
content/en/docs/collector/building/_index.md Outdated Show resolved Hide resolved
@hughesjj hughesjj force-pushed the add_component_description_to_building branch from 96e0d3e to f3e21f2 Compare April 25, 2024 18:32
Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Just some drive-by comments

content/en/docs/collector/building/_index.md Outdated Show resolved Hide resolved
content/en/docs/collector/building/_index.md Outdated Show resolved Hide resolved
Comment on lines 20 to 25
- `connectors` Ways to [connect pipelines](./connector/) and form a
[DAG](https://en.wikipedia.org/wiki/Directed_acyclic_graph) for your data
pipeline
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, I don't think people will really understand why having a DAG is helpful? I'd propose maybe something about "simplfying" the pipeline instead

Copy link
Author

Choose a reason for hiding this comment

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

That's fair, and is likely my own bias leaking. Perhaps

Connects the output (exporter) of one pipeline to the input (receiver) of another in a single component

I could also try making an analogy to unix pipes + stdout/stdin and/or golang channels. It's interesting -- I assume the audience for these docs has some sort of technical acumen (given these docs are all about creating components in otel), but not sure what the lingua franca/assumed background could be. Maybe channels would be the best analogy if we think analogies are worth while...

Copy link
Member

Choose a reason for hiding this comment

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

if you can improve that wording +1 for another example, but I personally think keeping it with DAG does no harm, 50%+ of our audience will understand it I guess

@hughesjj hughesjj requested review from a team as code owners May 1, 2024 23:07
@hughesjj hughesjj force-pushed the add_component_description_to_building branch from 67850ed to 846a66e Compare May 1, 2024 23:20
@hughesjj hughesjj force-pushed the add_component_description_to_building branch from bf47532 to 9ec3836 Compare May 3, 2024 23:57
@cartermp
Copy link
Contributor

cartermp commented May 5, 2024

/fix:format

@opentelemetrybot
Copy link
Contributor

You triggered fix:format action run at https://github.com/open-telemetry/opentelemetry.io/actions/runs/8959688934

@svrnm
Copy link
Member

svrnm commented May 6, 2024

@open-telemetry/collector-approvers PTAL!

@svrnm svrnm added the sig-approval-missing Co-owning SIG didn't provide an approval label May 6, 2024
content/en/docs/collector/building/_index.md Outdated Show resolved Hide resolved
typically used to convert data from an external source to OTLP
- `exporters`: Ways to
[export data](https://github.com/open-telemetry/opentelemetry-collector/tree/main/exporter#readme)
to non-OTLP formats, vendor-specific backends, or other Observability tools
Copy link
Member

Choose a reason for hiding this comment

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

not only non-OTLP: we do have OTLP exporters

Copy link
Author

@hughesjj hughesjj May 6, 2024

Choose a reason for hiding this comment

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

So, this was changed in a previous review to specifically mention non-otlp formats. I was presuming that was because only collector-core hosts the otlp exporter, but you're right in that even non-otlp exporters can export in the OTLP format, so will change. This also goes to your audience question. I think the goal is to be generic enough to cover all cases, but building your own collector via OCB and adding new components to -contrib are in scope for the audience AFAIK.

[augment the collector runtime](https://github.com/open-telemetry/opentelemetry-collector/blob/main/extension/README.md),
such as providing
[health checks](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/extension/healthcheckextension/README.md)
- `cmd`: Commands for building and/or maintain the collector
Copy link
Member

Choose a reason for hiding this comment

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

This should be removed, it's not relevant for people extending the collector.

Copy link
Author

@hughesjj hughesjj May 6, 2024

Choose a reason for hiding this comment

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

When you say "this", do you mean "remove information regarding extensions", or the health check specifically?

IMO it's relevant for people extending the collector, and if not, we should change the documentation which I read as indicating such

Extensions provide capabilities on top of the primary functionality of the collector
...
The contributors repository may have more extensions that can be added to custom builds of the Collector.

Also, we already have an article showing how to write an extension, so I think it would be a bit weird if we didn't mention extensions on the index of "building custom components"

Copy link
Member

@jpkrohling jpkrohling May 7, 2024

Choose a reason for hiding this comment

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

I mean specifically this item on the line I commented, cmd.

such as providing
[health checks](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/extension/healthcheckextension/README.md)
- `cmd`: Commands for building and/or maintain the collector
- `pkg`:
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, I don't think this should be included in this page.

Copy link
Author

Choose a reason for hiding this comment

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

I would appreciate a bit more elaboration on why you feel this way. I do think the cmd is a bit more esoteric, but I don't think that's a reason we should ignore its existence.

Copy link
Member

Choose a reason for hiding this comment

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

We don't have support for adding pkg in ocb at this point.

for adding functionality to the collector, such as support for
[golden tests](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/pkg/golden#readme)
or
[OTTL functions](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/pkg/ottl#readme)
Copy link
Member

Choose a reason for hiding this comment

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

Can we add an OTTL function by just adding a new package? It looks like it should be possible (cc @TylerHelmuth). However, we don't have an easy way to add a pkg to the a distribution right now. It would be a cool feature to have for the builder though.

or
[OTTL functions](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/pkg/ottl#readme)

Most components are registered using
Copy link
Member

Choose a reason for hiding this comment

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

This is only relevant if people are adding components to contrib. Perhaps this doc needs to be clearer on what's the target audience? Am I reading this because I'm developing a proprietary component to be used with my own distribution, used internally at my company? Or is this about developing an open-source component that is meant to be distributed as part of contrib? I would say that this doc is for the first case, and would expect people to check the contribution guidelines in contrib if they are looking for the second case.

Copy link
Author

Choose a reason for hiding this comment

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

Hey, good call, I'll work on making the audience more explicit.

We had a discussion and the current consensus as far as I understand it is

  1. should not be -contrib specific, but should definitely cover some aspects of contributing or building off of -contrib. This would mostly be in the form of linking to contrib documentation (similar to how we've linked to core documentation for receivers etc). Critically, we want to indicate that there's a high bar for ongoing maintenance to any -contrib contributions, to dissuade developers from "dumping" a new component in -contrib and then abandoning it, and to inform them of alternative ways to experiment with new components. That said, I do expect some SMEs who will eventually maintain constructive, useful components to come across this guide, and I don't want to completely scare them away either.
  2. OCB should be covered
  3. Forking -contrib should be covered (this wasn't explicitly called out, but I think it's an obvious place to start for most aspiring contributors)

Copy link
Member

Choose a reason for hiding this comment

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

They might end up in the contrib repository, but I argue that there are many use cases were folks want to have their "own" thing that might not even be OSS.

I'll let @svrnm confirm, but I believe the focus should be those many use cases where folks want to have their "own" thing. We can keep contrib off this doc, as it would make it a lot simpler, while still pointing people to the contrib-specific guidelines at that repo.

Copy link
Member

Choose a reason for hiding this comment

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

I thought about this, and sorry if this is different to what we have discussed before, but I lean towards "making it a lot simpler", so keeping things out of the docs that are contrib-specific. If we think that they would add additional value, we can still add them in later as well.

---

The OpenTelemetry Collector can not only be extended by existing components, but
also by custom components, that you develop and build on your own. Here you will
find instructions on how to build some of those components. For additional
details take a look into the documents contained within the
[opentelemetry-collector-contrib repository](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/README.md).

There are several
[classes](https://github.com/open-telemetry/opentelemetry-collector/blob/67d37183e6ac9b7180fefc6dc3a55f2a75c12fba/cmd/mdatagen/main.go#L192)
Copy link
Member

Choose a reason for hiding this comment

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

I would link to our documentation, I don't think a link to mdatagen's internals is the right approach here

Suggested change
[classes](https://github.com/open-telemetry/opentelemetry-collector/blob/67d37183e6ac9b7180fefc6dc3a55f2a75c12fba/cmd/mdatagen/main.go#L192)
[classes](/docs/collector/configuration/#basics)

Copy link
Author

@hughesjj hughesjj May 6, 2024

Choose a reason for hiding this comment

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

I agree linking to the internal code is suboptimal. I don't think linking to configuration is the right idea either though.

Should I add a blurb to the mdatagen readme to talk about what a "class" is? Or should I instead link to the metadata-schema.yaml file?

I think so long as we link to somewhere that enumerates all the classes, a reader could follow the breadcrumbs to more detailed documentation.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should link at mdatagen at all. The fact that we use mdatagen is an implementation detail: we do not require for you to use mdatagen to create a new component: you just need a go.mod and a properly-typed NewFactory function and the builder can take it.

Comment on lines +18 to +33
- `receivers`: Scrapers and Listeners which
[ingest data](https://github.com/open-telemetry/opentelemetry-collector/tree/main/receiver#readme),
typically used to convert data from an external source to OTLP
- `exporters`: Ways to
[export data](https://github.com/open-telemetry/opentelemetry-collector/tree/main/exporter#readme)
to non-OTLP formats, vendor-specific backends, or other Observability tools
- `processors`: Ways to
[process data](https://github.com/open-telemetry/opentelemetry-collector/tree/main/processor#readme)
in a pipeline
- `connectors`:
[Connects](https://github.com/open-telemetry/opentelemetry-collector/tree/main/connector#readme)
the output (exporter) of one pipeline to the input (receiver) of another in a
single component
- `extensions`: Ways to
[augment the collector runtime](https://github.com/open-telemetry/opentelemetry-collector/blob/main/extension/README.md),
such as providing
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

The intent is significantly different. Configuration is intended/written for aspiring users of the collector, while this guide is for aspiring contributors (or at least, developers). The introduction to the idea of different "classes" of components will be inherently similar though, yes.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, it's still unclear to me what the audience and intent of this doc is. Is it for people building their custom components? Is it a "how to add your component to contrib"? Is it "how to contribute to the opentelemetry-collector and/or opentelemetry-collector-contrib repositories"?

Comment on lines +37 to +41
[Packages](https://github.com/search?q=org%3Aopen-telemetry+%22class%3A+pkg%22&type=code)
for adding functionality to the collector, such as support for
[golden tests](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/pkg/golden#readme)
or
[OTTL functions](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/pkg/ottl#readme)
Copy link
Member

Choose a reason for hiding this comment

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

I am against promoting the usage of the pkg folder outside of contrib in our public documentation. I don't think we have figured out our strategy here yet

Copy link
Author

Choose a reason for hiding this comment

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

That's totally fair. Would you be okay with an aside saying something to the effect of

"While there may exist other component classes, at this time they are not in scope for this guide"
?

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

The current docs under docs/collector/building cover how to build custom components of different types. This PR seems to go in the direction of "how to contribute to opentelemetry-collector-contrib" instead. I don't think the opentelemetry.io is the right page to host that kind of content: that's what opentelemetry-collector-contrib's CONTRIBUTING.md is for, and it's going to be hard to keep this in sync with whatever the Collector SIG decides to do. IMO, opentelemetry.io docs folder should limit itself to "how to use the artifacts produced by the different OpenTelemetry SIGs" (in the case of the Collector that would be the Collector binaries, the Collector builder and the Collector libraries), not how to contribute to the SIG themselves.

@svrnm
Copy link
Member

svrnm commented Jun 11, 2024

@hughesjj @mx-psi can we get this PR into a mergable state, what's missing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig:collector sig-approval-missing Co-owning SIG didn't provide an approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants