Kiali 1906 Jaeger Integration by aljesusg · Pull Request #965 · kiali/kiali-ui

aljesusg · 2019-01-28T17:43:27Z

Jaeger Integration

Require: kiali/kiali#748
This PR add the feature to get the errorTraces of a service

Issue reference

KIALI-1906

Tasks

Screenshot

When jaeger is not configured

aljesusg · 2019-01-30T09:07:07Z

@jotak @xeviknal @lucasponce this is ready, there are so tests this is why we have 6000+ sorry :(

jotak · 2019-01-31T12:21:54Z

package.json

    "js-yaml": "3.12.0",
    "json-beautify": "1.0.1",
    "lodash": "4.17.11",
+    "logfmt": "^1.2.1",


Can you provide some information on how and why this library is used? If I understand correctly it's also used in jaeger, but I'm not sure if we really need it, what I can see is that searchOptions.tags is expected to be a string in "logfmt" format and then is converted to plain json string to pass to jaeger, but what I fail to see is where these tags come from and if we can avoid having them in that format in the first place.

Also, I think you should remove the caret in lib version, as we don't use it in general.

logfmt is used in src/actions/JaegerThunkActions.ts

@aljesusg can you also fix that in your new PR? (the caret version)

jotak · 2019-01-31T12:29:48Z

src/config/jaegerQuery.ts

+  }
+};
+
+export const JAEGER_QUERY = () => {


I'm not sure if ALL_CAPS is wanted here ... @mtho11 I think you provided some naming guidelines but I can't retrieve the mail / source , what do you think?

if we don't want ALL_CAPS in JAEGER_QUERY we should change ICONS too

So, icons was changed, this should be camel case too

Let me fix that in #1008

jotak · 2019-01-31T12:31:28Z

src/config/jaegerQuery.ts

+};
+
+export const JAEGER_QUERY = () => {
+  return deepFreeze(jaegerQuery) as typeof jaegerQuery;


By the way I'm not sure this has to be a function, I would imagine that deepFreeze has to be called only once and then provide the same immutable constant, no?

yes I followed

kiali-ui/src/config/Config.ts

Lines 129 to 131 in 33ff814

export const config = () => {

return deepFreeze(conf) as typeof conf;

};

yeah, not sure why config is like that, too.

jotak · 2019-01-31T12:38:43Z

src/components/JaegerToolbar/LookBack.tsx

+}
+
+export class LookBack extends React.PureComponent<LookBackProps, {}> {
+  lookBackOptions = {


The labels conventions here differ with what we have in similar dropdowns (it's fine IMO to have different values, but the names should be consistent) See in Config.ts : we capitalize only the first word ("Last 5 min" , "Last hour" ...). Also we have "Last day" instead of "Last 24 hours".

I set the same value that in Jaeger UI Toolbar, I don't see any problem to format like the others dropdowns

Why this wasn't done before merge?

Let me fix that in #1008

jotak · 2019-01-31T13:20:18Z

src/components/JaegerToolbar/TagsControl.tsx

+      <Card>
+        <Card.Title>
+          Values should be in the{' '}
+          <a href="https://brandur.org/logfmt" target="_blank">


we should add rel="noopener noreferrer" to any link leading to third-parties, it prevents potential undesired data leak . See https://www.jitbit.com/alexblog/256-targetblank---the-most-underestimated-vulnerability-ever/

ok, I didn't know. thanks

same here, why isn't it done?

jotak · 2019-01-31T13:23:28Z

src/components/JaegerToolbar/index.tsx

@@ -0,0 +1,181 @@
+import * as React from 'react';


Is there a reason to name that file index.tsx instead of JaegerToolbar.tsx ?

To make the imports more cleaner

import { JaegerToolbar} from 'components/JaegerToolbar';

I saw other projects that they use this like patternfly-react. We should have these things in the Code Guidelines.

Ok ... I don't like it so much because I usually find a component from its file name in my IDE (I have a shortcut for that in vscode). But if it's only me, no problem you can ignore.

There is not problem about we can set the same name and create a index inside components with all the exports and will be the same

jotak · 2019-01-31T13:27:44Z

src/services/Api.ts

 };

-const newRequest = <P>(method: HTTP_VERBS, url: string, queryParams: any, data: any, auth?: AuthToken) =>
+export const newRequest = <P>(method: HTTP_VERBS, url: string, queryParams: any, data: any, auth?: AuthToken) =>


Why is it exported? I don't see it used in a new place other than Api.ts

you are right, I was testing query the jaeger api to get errorTraces by the browser and I forgot remove it

still exported...

jotak

Huge and good work! Some comments after code review, I haven't tested locally yet.

aljesusg · 2019-01-31T13:55:49Z

src/components/JaegerToolbar/LookBack.tsx

+  mapStateToProps,
+  mapDispatchToProps
+)(LookBack);
+


For example here @jotak

What do you think about this @mtho11 ? I saw other components like https://github.com/kiali/kiali-ui/blob/3ec7c4d7048227e54d6523358bbc8888f86fca1e/src/components/GraphFilter/GraphFilter.tsx where we set mapStateToProps & mapDispatchToProps in the same file instead create a new file in /containers.

I think that could be cleaner if we set this in the same file, the developer can check this props in teh same file without having to go to containers folder and we would have fewer files

hhovsepy · 2019-02-01T09:54:05Z

Error loading graph, no logs in kiali side:

lucasponce · 2019-02-06T14:48:29Z

Probably good to rebase, I've got this error trying to test it:

lucasponce · 2019-02-06T14:50:04Z

Another issue: tab title show some errors but tab looks empty

aljesusg · 2019-02-06T14:51:18Z

I'll ping you when this is ready, actually is looking for service.namespace and this is not working with tha TRACING bug

aljesusg · 2019-02-06T14:52:56Z

Another issue: tab title show some errors but tab looks empty

If you inspect the code you will see the iframe with the service value to productpage.bookinfo if you change the bookinfo to default you will see the traces, this is related with the bug @lucasponce I am thinking about how to fix this in the frontend side. On the other hand I saw that we should change the breadcumb name to traces too

aljesusg · 2019-02-19T12:47:33Z

@lucasponce @hhovsepy @jotak Can you verify and check this?

lucasponce

LGTM
Agreement to consolidate this work and add modifications in following PRs.

aljesusg added the do not merge A PR is not ready to merge label Jan 28, 2019

aljesusg changed the title ~~[WiP] Kiali 1906~~ [WiP] Kiali 1906 Jaeger Integration Jan 28, 2019

aljesusg mentioned this pull request Jan 28, 2019

[POC] Jaeger Integration #888

Closed

aljesusg force-pushed the kiali_1906 branch from 0e450db to c6c600f Compare January 30, 2019 08:09

aljesusg changed the title ~~[WiP] Kiali 1906 Jaeger Integration~~ Kiali 1906 Jaeger Integration Jan 30, 2019

aljesusg force-pushed the kiali_1906 branch 2 times, most recently from 986c336 to 33ff814 Compare January 30, 2019 09:00

aljesusg removed the do not merge A PR is not ready to merge label Jan 30, 2019

aljesusg requested review from jotak, lucasponce and xeviknal January 30, 2019 18:10

jotak reviewed Jan 31, 2019

View reviewed changes

aljesusg commented Jan 31, 2019

View reviewed changes

aljesusg added the do not merge A PR is not ready to merge label Feb 4, 2019

aljesusg force-pushed the kiali_1906 branch from 0eec84f to 9a57ed5 Compare February 7, 2019 12:40

aljesusg force-pushed the kiali_1906 branch from 9b13636 to 05246ef Compare February 19, 2019 12:12

KIALI_1906 Make changes in the Kiali-UI to integrate Jaeger Components

33280a2

aljesusg force-pushed the kiali_1906 branch from 05246ef to 33280a2 Compare February 19, 2019 12:26

Add e.g.

481cb82

aljesusg force-pushed the kiali_1906 branch from 58a4a92 to 481cb82 Compare February 19, 2019 18:28

lucasponce removed the do not merge A PR is not ready to merge label Feb 19, 2019

lucasponce approved these changes Feb 19, 2019

View reviewed changes

lucasponce merged commit 31df0cc into kiali:master Feb 19, 2019

aljesusg deleted the kiali_1906 branch September 9, 2019 08:36

	export const config = () => {
	return deepFreeze(conf) as typeof conf;
	};

Conversation

aljesusg commented Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Jaeger Integration

Issue reference

Tasks

Screenshot

When jaeger is not configured

Uh oh!

aljesusg commented Jan 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jotak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hhovsepy commented Feb 1, 2019

Uh oh!

lucasponce commented Feb 6, 2019

Uh oh!

lucasponce commented Feb 6, 2019

Uh oh!

aljesusg commented Feb 6, 2019

Uh oh!

aljesusg commented Feb 6, 2019

Uh oh!

aljesusg commented Feb 19, 2019

Uh oh!

lucasponce left a comment

Choose a reason for hiding this comment

aljesusg commented Jan 28, 2019 •

edited

Loading