-
Notifications
You must be signed in to change notification settings - Fork 1.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
[docs/collector/building] add more context for what you can build #4343
[docs/collector/building] add more context for what you can build #4343
Conversation
96e0d3e
to
f3e21f2
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.
Just some drive-by comments
- `connectors` Ways to [connect pipelines](./connector/) and form a | ||
[DAG](https://en.wikipedia.org/wiki/Directed_acyclic_graph) for your data | ||
pipeline |
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 don't think people will really understand why having a DAG is helpful? I'd propose maybe something about "simplfying" the pipeline instead
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.
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...
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.
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
67850ed
to
846a66e
Compare
…may build or contribute to a collector
Co-authored-by: Fabrizio Ferri-Benedetti <[email protected]>
Co-authored-by: Fabrizio Ferri-Benedetti <[email protected]>
Co-authored-by: Severin Neumann <[email protected]>
Co-authored-by: Severin Neumann <[email protected]>
Co-authored-by: Severin Neumann <[email protected]>
Co-authored-by: Phillip Carter <[email protected]>
Co-authored-by: Phillip Carter <[email protected]>
bf47532
to
9ec3836
Compare
/fix:format |
You triggered fix:format action run at https://github.com/open-telemetry/opentelemetry.io/actions/runs/8959688934 |
@open-telemetry/collector-approvers PTAL! |
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 |
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 only non-OTLP: we do have OTLP 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.
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 |
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 should be removed, it's not relevant for people extending the collector.
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.
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"
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 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`: |
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.
Similarly, I don't think this should be included in this page.
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 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.
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 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) |
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.
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 |
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 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.
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.
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
- 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. - OCB should be covered
- 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)
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.
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.
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 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) |
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 would link to our documentation, I don't think a link to mdatagen's internals is the right approach here
[classes](https://github.com/open-telemetry/opentelemetry-collector/blob/67d37183e6ac9b7180fefc6dc3a55f2a75c12fba/cmd/mdatagen/main.go#L192) | |
[classes](/docs/collector/configuration/#basics) |
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 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.
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 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.
- `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 |
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 duplicates https://opentelemetry.io/docs/collector/configuration/#basics, doesn't 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.
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.
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.
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"?
[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) |
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 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
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.
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"
?
Co-authored-by: Juraci Paixão Kröhling <[email protected]>
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.
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.
1 similar comment
I don't think this content belongs in opentelemetry.io, I think we should close this PR and reopen on the opentelemetry-collector-contrib repository. |
As per discussion 3986, add some more context over what you may build in the collector (intends to cover #1 and #2 under building)