Skip to content

Add Jaeger links to the Linkerd dashboard#4177

Merged
alpeb merged 15 commits intolinkerd:masterfrom
Pothulapati:trace-board
May 7, 2020
Merged

Add Jaeger links to the Linkerd dashboard#4177
alpeb merged 15 commits intolinkerd:masterfrom
Pothulapati:trace-board

Conversation

@Pothulapati
Copy link
Contributor

@Pothulapati Pothulapati commented Mar 17, 2020

This PR

  • Adds support for reverse proxying jaeger through linkerd-web, allowing users to access jaeger UI at <linkerd-web>/jaeger

Signed-off-by: Tarun Pothulapati [email protected]

@Pothulapati
Add Jaeger reverse proxy
2349e1e 
Signed-off-by: Tarun Pothulapati <[email protected]>
@Pothulapati
update trace add-on golden files
65d53ae 
Signed-off-by: Tarun Pothulapati <[email protected]>
@Pothulapati Pothulapati mentioned this pull request Mar 18, 2020
Merged
@Pothulapati Pothulapati mentioned this pull request Mar 25, 2020
Merged
@Pothulapati
Copy link
Contributor Author

Pothulapati commented Apr 6, 2020

jaeger links will be available in the dashboard as seen in the below image
Screenshot_2020-04-04 Linkerd

@Pothulapati
add correct jaeger links based on resource types
b7495cd 
Signed-off-by: Tarun Pothulapati <[email protected]>
@Pothulapati
Merge branch 'master' into trace-board
a96eaf3 
Signed-off-by: Tarun Pothulapati <[email protected]>
@Pothulapati
update test golden files
d1f4948 
Signed-off-by: Tarun Pothulapati <[email protected]>
@Pothulapati
update MetricsTable tests
8dc7507 
Signed-off-by: Tarun Pothulapati <[email protected]>
@Pothulapati
Add optional jaeger link
2daff48 
Signed-off-by: Tarun Pothulapati <[email protected]>
@Pothulapati Pothulapati marked this pull request as ready for review April 29, 2020 09:21
@Pothulapati
fix namespace key tag
5a1b544 
Signed-off-by: Tarun Pothulapati <[email protected]>
@Pothulapati
Copy link
Contributor Author

Pothulapati commented Apr 29, 2020

To try this PR out:

  • Let config.yaml be
tracing:
  enabled: true

  • Install Linkerd by running
    linkerd install --control-plane-tracing --addon-config config.yaml | k apply -f -

  • Open linkerd-web dashboard and checkout the jaeger links in the MetricsTable of any component.

@admc admc requested a review from grampelberg May 6, 2020 16:12
@grampelberg
Copy link
Contributor

grampelberg commented May 6, 2020

@Pothulapati do we have the addons documented anywhere on the website?

grampelberg
grampelberg reviewed May 6, 2020
View reviewed changes
Copy link
Contributor

@grampelberg grampelberg left a comment

Choose a reason for hiding this comment

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

It works, and is awesome. I left some suggestions but nothing breaking, mostly minor nits.

@Pothulapati
Copy link
Contributor Author

Pothulapati commented May 6, 2020

@grampelberg I have some documentation here linkerd/website#686 specific to tracing add-on! WDYT?

@grampelberg
Copy link
Contributor

grampelberg commented May 6, 2020

@Pothulapati yup, that's what I was looking for. Thanks!

Pothulapati added 5 commits May 7, 2020 12:01
@Pothulapati
move query into separate fn
afe59e0 
Signed-off-by: Tarun Pothulapati <[email protected]>
@Pothulapati
update tests
f7c2c90 
Signed-off-by: Tarun Pothulapati <[email protected]>
@Pothulapati
rename grafana_proxy to reverse_proxy
f289d1d 
Signed-off-by: Tarun Pothulapati <[email protected]>
@Pothulapati
keep it DRY
76f5b59 
Signed-off-by: Tarun Pothulapati <[email protected]>
@Pothulapati
Merge branch 'master' into trace-board
6bbea43 
Signed-off-by: Tarun Pothulapati <[email protected]>
Pothulapati
Pothulapati commented May 7, 2020
View reviewed changes
web/app/js/components/MetricsTable.jsx
metrics: PropTypes.arrayOf(processedMetricsPropType),
resource: PropTypes.string.isRequired,
selectedNamespace: PropTypes.string.isRequired,
jaeger: PropTypes.string,
Copy link
Contributor Author

@Pothulapati Pothulapati May 7, 2020

Choose a reason for hiding this comment

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

There is a bit of confusion here as grafana is marked as isRequired while jaeger is not. As both of these values are passed as empty strings when the arg is empty, This should not cause any problems!

The change would be to remove isRequired for grafana and update the tests as they expect grafana field to be there always.

Copy link
Contributor Author

@Pothulapati Pothulapati May 7, 2020

Choose a reason for hiding this comment

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

raised #4349 to track

@kleimkuhler kleimkuhler mentioned this pull request May 7, 2020
Merged
grampelberg
grampelberg approved these changes May 7, 2020
View reviewed changes
Copy link
Contributor

@grampelberg grampelberg left a comment

Choose a reason for hiding this comment

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

:shipit:

@Pothulapati Pothulapati mentioned this pull request May 7, 2020
Closed
adleong
adleong approved these changes May 7, 2020
View reviewed changes
Copy link
Member

@adleong adleong 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 very cool! There are some confusing bits to it that we'll need to make sure we adequately document, either directly in the UI as a follow-up, or on the website. Specifically:

  • Enabling control plane tracing causes the jaeger icon to show up for all namespaces, even those without tracing, which can be confusing. Especially since some people have the notion that a service mesh can add tracing "for free" this could encourage that misunderstanding. It would be super cool if we could query jaeger and then only show the jaeger icon if any traces exist for that namespace.
  • Our tracing tutorial has users set up a separate jaeger from the linkerd-jaeger. This means that if you follow the tracing tutorial and also enable control plane tracing then you'll have emojivoto traces, but they won't be in linkerd's jaeger, and they'll be missing from this dashboard. We need to explain this and/or update the tutorial to use linkerd-jaeger.
  • The above two points taken together means that you can enable tracing in emojivoto according to the tutorial AND get a jaeger icon for the emojivoto namespace but not see any traces there which is certainly going to be confusing and frustrating for users.

@grampelberg
Copy link
Contributor

grampelberg commented May 7, 2020

@Pothulapati would you mind getting an issue to track the docs updates? @adleong has some great points!

This was referenced May 7, 2020
Closed
Closed
@Pothulapati
Copy link
Contributor Author

Pothulapati commented May 7, 2020

@grampelberg @adleong

Created linkerd/website#720 & #4351 to track them :)

alpeb
alpeb reviewed May 7, 2020
View reviewed changes
Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

Thanks @Pothulapati , looks good! 👍
One nit though: the jaeger icon svg is huge! 180k vs for example Grafana's which is just 5.4k

@grampelberg
Copy link
Contributor

grampelberg commented May 7, 2020

@zaharidichev mind giving this a review? It appears that you're a unique code owner snowflake for files that I'm not 100% sure of =)

alpeb
alpeb approved these changes May 7, 2020
View reviewed changes
Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

Ok LGTM. @Pothulapati please address the icon issue as a separate PR 😉

@alpeb alpeb merged commit 2be43a5 into linkerd:master May 7, 2020
@alpeb alpeb mentioned this pull request May 7, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alpeb alpeb alpeb approved these changes

@adleong adleong adleong approved these changes

@zaharidichev zaharidichev Awaiting requested review from zaharidichev

+1 more reviewer

@grampelberg grampelberg grampelberg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Pothulapati @grampelberg @alpeb @adleong