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

Update Tracing integration documentation #203

Merged
merged 19 commits into from Feb 18, 2020
Merged

Conversation

@aljesusg
Copy link
Contributor

aljesusg commented Feb 11, 2020

Fix kiali/kiali#2200

Update images and docs /documentation/distributed-tracing/

@aljesusg aljesusg requested a review from xeviknal Feb 11, 2020
@aljesusg aljesusg added this to In Review in Sprint 35 via automation Feb 11, 2020
Copy link
Member

xeviknal left a comment

Good! I have some comments though. And a question, before closing kiali/kiali#2200, should the FAQ be updated also?

content/documentation/distributed-tracing/index.adoc Outdated Show resolved Hide resolved
content/documentation/distributed-tracing/index.adoc Outdated Show resolved Hide resolved

In each service we'll have a new tab to display the traces for the service in a scatter plot. (If we have error traces this tab will display them)

This comment has been minimized.

Copy link
@xeviknal

xeviknal Feb 12, 2020

Member

Once the in_cluster_url setting is set on the Kiali CR, each service page will have the new Traces tab where a scatter plot will be displayed with its related traces.

I would avoid the usage of we.


When Kiali and Tracing service have the same protocol, we'll have the embedded feature in the website.
When Tracing url is set, Kiali show a link to the Distributed Tracing in the menu.

This comment has been minimized.

Copy link
@xeviknal

xeviknal Feb 12, 2020

Member

Once the url setting is set on the Kiali CR, Kiali will show...

content/documentation/distributed-tracing/index.adoc Outdated Show resolved Hide resolved
content/documentation/distributed-tracing/index.adoc Outdated Show resolved Hide resolved
@jotak

This comment has been minimized.

Copy link
Contributor

jotak commented Feb 17, 2020

@aljesusg @xeviknal @lucasponce I'm trying to reformulate documentation to better guide the user through configuration - so, having a dedicated Configuration section. What do you think about this?:

Distributed tracing

(HERE keep introduction on tracing...)

Configuration

In cluster URL

In order to fetch data from Jaeger, Kiali needs to get an URL that can be resolved from inside the cluster, typically using Kubernetes DNS. This is the in_cluster_url configuration. For instance, for a Jaeger service named 'tracing' within 'istio-system' namespace, Kiali config would be:

  external_services:
    tracing:
      in_cluster_url: 'http://tracing.istio-system/jaeger'

!NOTE! If you use the Kiali operator (recommended), this config can be set in the Kiali CR. But in most cases, the Kiali operator will set a valid default in_cluster_url so you wouldn't have to change anything. If you don't use the Kiali operator, this config can be set in Kiali config map.

-- screenshots to tracing-enabled Kiali --

External URL

Configuring an external URL for Jaeger will enable more links from Kiali to Jaeger UI. This URL needs to be accessible from the browser (it's used for links generation). Example:

  external_services:
    tracing:
      in_cluster_url: 'http://tracing.istio-system/jaeger'
      url: 'http://my-jaeger-host'

-- screenshots to external links --

!NOTE! You may have url configured and not in_cluster_url, for instance, if Jaeger is not accessible from Kiali pod. In this situation, Kiali will not show its own traces chart but will display external links to the Jaeger UI instead.

Other configuration

For advanced configuration on Jaeger integration, please refer to the Kiali CR description. It is relevant for config map as well, if you don't use the Kiali operator.

etc.

(THEN follow on features description - assuming Kiali is fully configured for tracing)

@xeviknal

This comment has been minimized.

Copy link
Member

xeviknal commented Feb 17, 2020

I like the proposal. I think it will help people better understand how to config the integration.

@aljesusg

This comment has been minimized.

Copy link
Contributor Author

aljesusg commented Feb 17, 2020

I agree but the oproblem here is that we have a FAQ about tracing, another page with the information about tracing and another one for the config..

3 places about tracing information !!! without links to each other so the user must check the 3 views. we need to reorganize the website

@jotak

This comment has been minimized.

Copy link
Contributor

jotak commented Feb 17, 2020

@aljesusg what is the page about configuration? I can't find that...

About FAQ, it's a different approach, focused on troubleshooting / Q&A. It answers very specific problems, not like here. Maybe some items in FAQ should be rewritten, I haven't checked yet.

FAQ to me is more "the documentation of the last chance" ; when stuff doesn't work as expected and I can't figure out why. Ideally I should never have to check a FAQ, as a user... (this is how I see it, maybe it's just me)

@aljesusg

This comment has been minimized.

Copy link
Contributor Author

aljesusg commented Feb 17, 2020

@aljesusg what is the page about configuration? I can't find that...

mmm maybe we removed that page I can't find it neither

About FAQ, it's a different approach, focused on troubleshooting / Q&A. It answers very specific problems, not like here. Maybe some items in FAQ should be rewritten, I haven't checked yet.

url: 'http://my-jaeger-host'
```

Once the in_cluster_url setting is set on the Kiali CR, each service page will have the new Traces tab where a scatter plot will be displayed with its related traces.

This comment has been minimized.

Copy link
@jotak

jotak Feb 17, 2020

Contributor

This sentence better describes the previous section (in cluster URL)

@jotak

This comment has been minimized.

Copy link
Contributor

jotak commented Feb 17, 2020

Thanks @aljesusg .
I'm not sure why the deployment failed ; i cannot see any logs, maybe some temporary issue with netlify

@aljesusg aljesusg force-pushed the aljesusg:fix_2200 branch from 17d8068 to 50dadf7 Feb 18, 2020
Copy link
Contributor

jotak left a comment

Sending some suggestions that we can discuss. Most notably, after checking on deployed site I think screenshots of Traces screen are better placed outside of the configuration section (so I created a new section).
Else, there's some rephrasing and a couple of typos

content/documentation/distributed-tracing/index.adoc Outdated Show resolved Hide resolved
content/documentation/distributed-tracing/index.adoc Outdated Show resolved Hide resolved
content/documentation/distributed-tracing/index.adoc Outdated Show resolved Hide resolved
content/documentation/distributed-tracing/index.adoc Outdated Show resolved Hide resolved
content/documentation/distributed-tracing/index.adoc Outdated Show resolved Hide resolved
content/documentation/distributed-tracing/index.adoc Outdated Show resolved Hide resolved
content/documentation/distributed-tracing/index.adoc Outdated Show resolved Hide resolved
content/documentation/distributed-tracing/index.adoc Outdated Show resolved Hide resolved
content/documentation/distributed-tracing/index.adoc Outdated Show resolved Hide resolved
aljesusg and others added 10 commits Feb 18, 2020
Co-Authored-By: Joel Takvorian <joel.takvorian@qaraywa.net>
Co-Authored-By: Joel Takvorian <joel.takvorian@qaraywa.net>
Co-Authored-By: Joel Takvorian <joel.takvorian@qaraywa.net>
Co-Authored-By: Joel Takvorian <joel.takvorian@qaraywa.net>
Co-Authored-By: Joel Takvorian <joel.takvorian@qaraywa.net>
Co-Authored-By: Joel Takvorian <joel.takvorian@qaraywa.net>
Co-Authored-By: Joel Takvorian <joel.takvorian@qaraywa.net>
Co-Authored-By: Joel Takvorian <joel.takvorian@qaraywa.net>
Co-Authored-By: Joel Takvorian <joel.takvorian@qaraywa.net>
Co-Authored-By: Joel Takvorian <joel.takvorian@qaraywa.net>
@jotak

This comment has been minimized.

Copy link
Contributor

jotak commented Feb 18, 2020

@aljesusg , not sure if you saw it, there's a couple more suggestions - probably github is hidding them to you:

Capture d’écran de 2020-02-18 12-14-16

aljesusg and others added 2 commits Feb 18, 2020
Co-Authored-By: Joel Takvorian <joel.takvorian@qaraywa.net>
Co-Authored-By: Joel Takvorian <joel.takvorian@qaraywa.net>
aljesusg and others added 6 commits Feb 18, 2020
Co-Authored-By: Joel Takvorian <joel.takvorian@qaraywa.net>
Co-Authored-By: Joel Takvorian <joel.takvorian@qaraywa.net>
Co-Authored-By: Joel Takvorian <joel.takvorian@qaraywa.net>
Co-Authored-By: Joel Takvorian <joel.takvorian@qaraywa.net>
Co-Authored-By: Joel Takvorian <joel.takvorian@qaraywa.net>
Co-Authored-By: Joel Takvorian <joel.takvorian@qaraywa.net>
@jotak
jotak approved these changes Feb 18, 2020
Copy link
Contributor

jotak left a comment

LGTM thanks @aljesusg !

@aljesusg

This comment has been minimized.

Copy link
Contributor Author

aljesusg commented Feb 18, 2020

thank for you great review @jotak

@aljesusg aljesusg merged commit 7199d81 into kiali:master Feb 18, 2020
5 checks passed
5 checks passed
Header rules - kiali No header rules processed
Details
Pages changed - kiali All files already uploaded
Details
Redirect rules - kiali No redirect rules processed
Details
Mixed content - kiali No mixed content detected
Details
netlify/kiali/deploy-preview Deploy preview ready!
Details
Sprint 35 automation moved this from In Review to Done Feb 18, 2020
@aljesusg aljesusg deleted the aljesusg:fix_2200 branch Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Sprint 35
  
Done
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.