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

Move Collector architecture documentation and diagrams from Github to opentelemetry.io #4029

Merged
merged 24 commits into from
Mar 7, 2024

Conversation

tiffany76
Copy link
Contributor

Fixes #3547.

This PR creates a new docs page (Architecture) in the Collector documentation on opentelemetry.io. The content is being moved from Github (docs/design.md) to the OTel website to supplement existing documentation.

The file has been copyedited and modified to fit website heading conventions.

The diagram image files have been replaced in favor of Mermaid diagrams.

(Note: Work to remove outdated content and merge what remains with existing opentelemetry.io pages will be handled in a future issue/PR.)

Copy link

linux-foundation-easycla bot commented Feb 20, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Comment on lines 81 to 82
For details of config file format see
[this document](https://docs.google.com/document/d/1NeheFG7DmcUYo_h2vLtNRlia9x5wOJMlV4QKEK05FhQ/edit#).
Copy link
Member

Choose a reason for hiding this comment

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

why is this in a google doc and not in a markdown file in the collector repo? I guess this has historical reasons, maybe we can migrate it as well?

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'm not sure about the document's origin, but I'm happy to convert it to Markdown and move it to the Collector repo if that's the consensus.

Copy link
Member

Choose a reason for hiding this comment

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

Good question where this should live, this is up to the @open-telemetry/collector-approvers

Copy link
Member

Choose a reason for hiding this comment

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

I'd remove it altogether. It's so old that a few things are already wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in 2740396. Thanks!

Copy link
Member

@svrnm svrnm left a comment

Choose a reason for hiding this comment

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

This is a really great start, we might need to do some passes to make this content up-to-date.

@svrnm
Copy link
Member

svrnm commented Feb 21, 2024

@tiffany76 no need to keep this as a draft from my point of view! This will also trigger some review requests with @open-telemetry/collector-approvers who should definitely provide an early review.

Co-authored-by: Severin Neumann <severin.neumann@altmuehlnet.de>
@tiffany76 tiffany76 marked this pull request as ready for review February 21, 2024 18:49
@tiffany76 tiffany76 requested review from a team and bogdandrutu and removed request for a team February 21, 2024 18:49
traces: # a pipeline of “traces” type
receivers: [otlp]
processors: [memory_limiter, batch]
exporters: [jaeger]
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 count 11 more mentions of jaeger in the document, including several in diagrams (two of which I'm still working on).

Copy link
Member

Choose a reason for hiding this comment

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

We do not need to remove all of them, only the instances where the jaeger exporter is used since it has been deprecated a while ago already, the receivers are still fine, but we still could consider using OTLP there. Jaeger can remain if named as a potential backend (which now supports OTLP)

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 changed mentions of jaeger exporter to otlp in 8e12f26.

Copy link
Member

@theletterf theletterf left a comment

Choose a reason for hiding this comment

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

Nice work, thank you! Just some suggestions and nits.

content/en/docs/collector/architecture.md Outdated Show resolved Hide resolved
content/en/docs/collector/architecture.md Outdated Show resolved Hide resolved
content/en/docs/collector/architecture.md Outdated Show resolved Hide resolved
content/en/docs/collector/architecture.md Outdated Show resolved Hide resolved
content/en/docs/collector/architecture.md Outdated Show resolved Hide resolved
content/en/docs/collector/architecture.md Outdated Show resolved Hide resolved
content/en/docs/collector/architecture.md Outdated Show resolved Hide resolved
content/en/docs/collector/architecture.md Outdated Show resolved Hide resolved
content/en/docs/collector/architecture.md Outdated Show resolved Hide resolved
content/en/docs/collector/architecture.md Outdated Show resolved Hide resolved
@tiffany76
Copy link
Contributor Author

/fix:format

Copy link
Contributor

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

@mx-psi
Copy link
Member

mx-psi commented Feb 29, 2024

Is there a preview on how this will look on the opentelemetry.io website?

@tiffany76
Copy link
Contributor Author

tiffany76 commented Feb 29, 2024

@mx-psi Try this one.

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

I didn't finish reviewing yet, but here are a few early comments.

Comment on lines 81 to 82
For details of config file format see
[this document](https://docs.google.com/document/d/1NeheFG7DmcUYo_h2vLtNRlia9x5wOJMlV4QKEK05FhQ/edit#).
Copy link
Member

Choose a reason for hiding this comment

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

I'd remove it altogether. It's so old that a few things are already wrong.

content/en/docs/collector/architecture.md Outdated Show resolved Hide resolved

> The configuration uses composite key names in the form of `type[/name]` as
> defined in
> [this document](https://docs.google.com/document/d/1NeheFG7DmcUYo_h2vLtNRlia9x5wOJMlV4QKEK05FhQ/edit#)).
Copy link
Member

Choose a reason for hiding this comment

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

Again, I'd remove mentions to this file. While it has parts that clarify this particular trait, other parts of the doc are outdated and might confuse users.

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've removed mention of the file (2740396) but left the config explanation.

flowchart LR
R1("`#quot;opentelemetry-collector#quot; Receiver`") --> FO((fan-out))
FO -->|Pipeline 'traces'| P1["`#quot;memory_limiter#quot; Processor`"]
FO -->|Pipeline 'traces/2'| P2["`#quot;tags#quot; Processor`"]
Copy link
Member

Choose a reason for hiding this comment

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

What's this tags processor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 'tags' processor is mentioned in two diagrams from the original document: the receiver diagram and the exporter diagram.

I've recreated them as Mermaid diagrams, so we can make changes easily if the 'tags' processor is no longer valid or relevant.

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 remember about that processor, and I would expect this diagram to be related to the config that was shared in the previous example.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can say 'transform' instead of tags here?

Copy link
Member

Choose a reason for hiding this comment

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

The collector config coming right before this would need to be updated as well. I believe this diagram here is a representation of that config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated configs and diagrams to replace tags with transform: ff7579f.

@jpkrohling
Copy link
Member

I'm missing "Connectors" here, and I think @djaglowski's talk at KubeCon Amsterdam was great in that area, where he not only mentioned how connectors work, but how they differ from the collector on seemingly identical features (same receiver on two pipelines vs. forward connector, I believe). It might be good to incorporate that, or at least link the presentation there.

https://www.youtube.com/watch?v=uPpZ23iu6kI

@svrnm
Copy link
Member

svrnm commented Mar 5, 2024

I'm missing "Connectors" here, and I think @djaglowski's talk at KubeCon Amsterdam was great in that area, where he not only mentioned how connectors work, but how they differ from the collector on seemingly identical features (same receiver on two pipelines vs. forward connector, I believe). It might be good to incorporate that, or at least link the presentation there.

https://www.youtube.com/watch?v=uPpZ23iu6kI

I wonder if we should do this in a follow up PR?, so step 1 is to bring over that content from the collector repo to docs, step2 is removing that file from the collector repo, step3 is some additional scrubbing of this file (incl. adding some words about connectors)

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.

Agree with Severin that we can move forward with this PR and leave connectors to a follow up PR.

flowchart LR
R1("`#quot;opentelemetry-collector#quot; Receiver`") --> FO((fan-out))
FO -->|Pipeline 'traces'| P1["`#quot;memory_limiter#quot; Processor`"]
FO -->|Pipeline 'traces/2'| P2["`#quot;tags#quot; Processor`"]
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can say 'transform' instead of tags here?

@tiffany76 tiffany76 requested review from jpkrohling, svrnm and mx-psi March 7, 2024 01:46
Copy link
Member

@svrnm svrnm left a comment

Choose a reason for hiding this comment

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

I am happy with this as is, we will need to do some follow up work on it, but I prefer merging this in and then incrementally improve.

@cartermp cartermp merged commit 7996562 into open-telemetry:main Mar 7, 2024
14 checks passed
@tiffany76 tiffany76 deleted the collector_architecture branch March 7, 2024 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[/collector] Add diagrams from Collector's docs/design.md to Configuration and Deployment pages
6 participants