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

Restructure distributed tracing #61948

Closed
wants to merge 13 commits into from

Conversation

pavolloffay
Copy link
Member

@pavolloffay pavolloffay commented Jun 30, 2023

Version(s):
4.10+

Issue:

https://issues.redhat.com/browse/TRACING-3288

Link to docs preview:

https://61948--docspreview.netlify.app/

QE review:

  • QE has approved this change.

Additional information:

There is no documentation content change in this PR, only structure was changed.

This PR splits Distributed tracing/Distributed tracing installation into:

  • Distributed tracing/Distributed tracing platform - Jaeger
  • Distributed tracing/Distributed tracing data collection - OpenTelemetry

The spit is necessary because we want to introduce Distributed tracing/Distributed tracing platform - Tempo and it will be better to have documentation per component. In the future we will probably move the Distributed tracing data collection - OpenTelemetry into the root level as it might become a separate product.

Each of these components will host following sub articles:

  • instaling
  • configuring
  • updating
  • removing

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 30, 2023
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Jun 30, 2023

🤖 Updated build preview is available at:
https://61948--docspreview.netlify.app

Build log: https://circleci.com/gh/ocpdocs-previewbot/openshift-docs/19169

@openshift-ci openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 30, 2023
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
@openshift-ci openshift-ci bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 30, 2023
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
@openshift-ci openshift-ci bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 30, 2023
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
@openshift-ci openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 30, 2023
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
@pavolloffay pavolloffay changed the title WIP: Restructure distributed tracing Restructure distributed tracing Jun 30, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 30, 2023
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
@@ -3576,19 +3576,24 @@ Topics:
Topics:
- Name: Distributed tracing architecture
Copy link
Member

Choose a reason for hiding this comment

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

This could be in the same level as Distributed tracing release notes Currently we have to click on the Distributed tracing architecture dropdown arrow and then again select Distributed tracing architecture to view the page.

Copy link
Member Author

Choose a reason for hiding this comment

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

agree, but I consider this out of the scope of this work. I will open a separate PR to fix this

File: distr-tracing-updating
- Name: Removing distributed tracing
File: distr-tracing-removing
- Name: Distributed tracing platform - Jaeger
Copy link
Member

Choose a reason for hiding this comment

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

Should we use camel case for the topic names ? Like Distributed Tracing Platform - Jaeger and Distributed Tracing Data Collection - OpenTelemetry.

Copy link
Member Author

Choose a reason for hiding this comment

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

The official name of the product is Red Hat OpenShift distributed tracing platform (lower case after OpenShift), but here I guess we can use capitalization as we want.

I will leave this to docs team to decide

@IshwarKanse
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 3, 2023
@andreasgerstmayr
Copy link
Contributor

/lgtm

:context: install-distributed-tracing-otel

toc::[]

Copy link
Contributor

@max-cx max-cx Jul 4, 2023

Choose a reason for hiding this comment

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

pavolloffay, I'm not acquainted with this product. I noticed that this page is missing the following paragraphs, see another file in this PR, which are in the preview.
Just to double-check, this is a deliberate omission, correct? Or are these paragraphs not relevant to the content on this page?
I'm talking about these paragraphs:
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, and this was a mistake in docs. The OpenTelemetry does not integrate with service mesh in any form. The service mesh docs do not contain anything about the OTEL operator.

Copy link
Contributor

@max-cx max-cx Jul 4, 2023

Choose a reason for hiding this comment

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

BTW, just curious: what's your plan regarding the content in https://docs.openshift.com/container-platform/4.13/service_mesh/v2x/ossm-reference-jaeger.html? Will it remain there or be eventually moved under distr_tracing_jaeger?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is a question for the service mesh team. I think in the past they wanted to have Jaeger installation docs directly in their docs, however, they are recently trying to decouple from 3rd party tools, so they will maybe just send user to jaeger docs.

@@ -1,7 +1,7 @@
////
Module included in the following assemblies:
* distr_tracing/distr_tracing_install/distr-tracing-deploying-jaeger.adoc
* distr_tracing/distr_tracing_install/distr-tracing-deploying-otel.adoc
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please double-check this reference. It didn't seem up to date to me:

* distr_tracing/distr_tracing_install/distr-tracing-deploying-otel.adoc


toc::[]

The {OTELName} Operator uses a custom resource definition (CRD) file that defines the architecture and configuration settings to be used when creating and deploying the {OTELName} resources. You can either install the default configuration or modify the file to better suit your business requirements.
The {OTELName} Operator uses a custom resource definition (CRD) file that defines the architecture and configuration settings to be used when creating and deploying the {OTELName} resources. You can either install the default configuration or modify the file to better suit your business requirements.

include::modules/distr-tracing-config-otel-collector.adoc[leveloffset=+1]
Copy link
Contributor

@max-cx max-cx Jul 4, 2023

Choose a reason for hiding this comment

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

I suggest removing the other, commented out include on the following line in another file: https://github.com/openshift/openshift-docs/pull/61948/files#diff-ede8739ac256226eee0c61eab7a1c9b1b216c85b83ec3e5088cbc809004873f6R77.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

_topic_maps/_topic_map.yml Outdated Show resolved Hide resolved
_topic_maps/_topic_map.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@max-cx max-cx left a comment

Choose a reason for hiding this comment

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

@pavolloffay, I left a few more comments here. Please review them. Pre-approving this PR.

Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 4, 2023
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
@IshwarKanse
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 5, 2023
@IshwarKanse
Copy link
Member

/lgtm

max-cx added a commit to max-cx/openshift-docs that referenced this pull request Jul 14, 2023
…s into distributed-tracing-2.9

Merging changes from Pavol's PR openshift#61948
@max-cx max-cx mentioned this pull request Jul 14, 2023
1 task
@max-cx
Copy link
Contributor

max-cx commented Jul 14, 2023

@pavolloffay, fyi, I've merged your changes from this PR into PR #62410.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 23, 2023
@openshift-merge-robot
Copy link

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@pavolloffay
Copy link
Member Author

It will be done as part of #62410

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants