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: explain Jaeger and collector bundling in getting started guide #3743

Closed
wants to merge 6 commits into from

Conversation

Garvit-77
Copy link

@Garvit-77 Garvit-77 commented Oct 8, 2024

Summary

According to the DoD(3 Definitions) the changes were made.
Made Changes in 2 Documents namely getting started and otel(guides)

The motivation was to provide a Clear Documentation and a better understand of the concept of collectors to the users of Keptn and Jaeger.

This addresses an issue raised on Slack (link), where inconsistencies were found between the "Getting Started" guide and the main Jaeger documentation about how the OpenTelemetry collector is used. The response by @agardnerIT clarified the confusion, and the changes ensure this information is reflected in the Keptn documentation.

Fixes #3538

Checks

  • My PR fulfills the Definition of Done of the corresponding issue and not more (or parts if the issue is separated
    into multiple PRs)
  • I used descriptive commit messages to help reviewers understand my thought process
  • I signed off all my commits according to the Developer Certificate of Origin (DCO)(
    see Contribution Guide)
  • My PR title is formatted according to the semantic PR conventions described in
    the Contribution Guide
  • My content follows the style guidelines of this project (YAMLLint, markdown-lint)
  • I regenerated the auto-generated docs for Helm and the CRD documentation (if applicable)
  • I have performed a self-review of my content including grammar and typo errors and also checked the rendered page
    from the Netlify deploy preview
  • My changes result in all-green PR checks (first-time contributors need to ask a maintainer to approve their test runs)

@Garvit-77 Garvit-77 requested review from a team as code owners October 8, 2024 12:29
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Oct 8, 2024
@Garvit-77
Copy link
Author

Please review and suggest changes if required
Open for feedback

@mowies mowies changed the title Otel #3538 docs: fix some inconsistencies Oct 9, 2024
@mowies mowies changed the title docs: fix some inconsistencies docs: explain that the OTel collector comes bundled with Jaeger in the getting started guide Oct 9, 2024
@mowies mowies changed the title docs: explain that the OTel collector comes bundled with Jaeger in the getting started guide docs: explain Jaeger and collector bundling in getting started guide Oct 9, 2024
Copy link
Member

@mowies mowies left a comment

Choose a reason for hiding this comment

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

I think this issue can be solved with much less documentation. Pls see my other comments. Sorry for the inconvenience here 😅

@@ -208,3 +208,25 @@ of your application and apply the`KeptnAppContext`.
Keptn will re-deploy your application and Jaeger should show a link to the previous trace in the references section.

![linked trace](./assets/linkedtrace.png)

## OTEL Internal Data Processing
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this whole section, it's not necessary to explain, how the OTel collector works. People can read good documentation about it in the OTel docs that are linked already.

Copy link
Author

Choose a reason for hiding this comment

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

@mowies in the DoD it's mentioned to

  1. Detailed explanation in the Otel guide page about how the otel collector wrks in case you do not want to use Jaeger allInOne.

Copy link
Member

Choose a reason for hiding this comment

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

That is not needed. People can read about how the collector works in the OTel docs. No need to document this on our side.

docs/docs/getting-started/observability.md Outdated Show resolved Hide resolved
docs/docs/getting-started/observability.md Outdated Show resolved Hide resolved
docs/docs/getting-started/observability.md Outdated Show resolved Hide resolved
@@ -282,8 +281,14 @@ So let's install new Observability components to help us:
Scrape metrics from the above DORA metrics endpoint & forward to Prometheus
- [Grafana](https://grafana.com) (and some prebuilt dashboards): Visualise the data

![add observability](./assets/install01.png)
**Note**
Copy link
Member

Choose a reason for hiding this comment

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

Instead of this note, please reorder the items above so that the OTel collector is right below Jaeger and then add in parentheses:

(in this guide, we will use the OTel collector that comes bundled with the Jaeger `allInOne` image)

mowies and others added 2 commits October 9, 2024 10:55
Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com>
@mowies
Copy link
Member

mowies commented Oct 16, 2024

@Garvit-77 any updates here?

@Garvit-77
Copy link
Author

Hello @mowies
I have completed the issue but don't know why I'm not able to update the PR
I will resolve this by tonight and update the PR

Copy link

sonarcloud bot commented Oct 18, 2024

@Garvit-77 Garvit-77 closed this Oct 18, 2024
@mowies
Copy link
Member

mowies commented Oct 21, 2024

@Garvit-77 was this closed on purpose?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify how otel data is processed internally
2 participants