Skip to content
This repository was archived by the owner on Apr 11, 2023. It is now read-only.

KIALI-2456,2459,2466,2465, 2450 and 2507 . Improve toolbar for Jaeger Integration and refactor#1025

Merged
lucasponce merged 2 commits intokiali:masterfrom
aljesusg:k_2456
Mar 12, 2019
Merged

KIALI-2456,2459,2466,2465, 2450 and 2507 . Improve toolbar for Jaeger Integration and refactor#1025
lucasponce merged 2 commits intokiali:masterfrom
aljesusg:k_2456

Conversation

@aljesusg
Copy link
Copy Markdown
Contributor

@aljesusg aljesusg commented Feb 26, 2019

  • KIALI-2456 Error Traces shows kiali home when refreshing the page

  • KIALI-2459 Fix traces tab if value is -1

  • KIALI-2466 Remove visual options in jaeger toolbar (Hide graph, hide summary) and make the search button bigger

  • KIALI-2465 Get Services in jaegerToolbar is not working after Openshift oauth changes

  • KIALI-2450 Change the namespace Selector in Jaeger Distributed tracing view to be like other views

  • KIALI-2507 Distributed tracing link disappear in a new tab

  • KIALI-2515 UI About - Components not persist when the user open a new tab

  • All removed from redux to improve performance

  • Now when you click lookback custom by default is set the dates from 1 hour

  • Now we support query params in the jaeger integration
    Screenshot from 2019-03-11 09-34-40
    Peek 2019-03-11 09-39

Comment thread src/store/ConfigStore.ts Outdated
key: persistKey,
storage: storage,
whitelist: ['authentication', 'namespaces', 'serverConfig'],
whitelist: ['authentication', 'namespaces', 'serverConfig', 'jaegerState'],
Copy link
Copy Markdown
Contributor

@josejulio josejulio Feb 26, 2019

Choose a reason for hiding this comment

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

Should we store all the jaegerState? I'm worried about storing information that might not be accurate the next time we use Kiali such as: toolbar.isFetchingService and jaegerURL.

Please, if needed, consider to only store a subset of this, see: https://github.com/kiali/kiali-ui/blob/master/src/store/ConfigStore.ts#L30-L41

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Totally agree , I am still testing why the state of redux take time in get updated before the rendering of the component, We'll only persist the jaegerUrl.

@aljesusg
Copy link
Copy Markdown
Contributor Author

@hhovsepy I fixed the problem here, now traces tab is hide until load. But I see another problems like the missing sidecar icon (See the gif)

persiststore

Maybe we should create some loading view while we fetch the backend what do you think @lucasponce @jotak @xeviknal ?

@aljesusg
Copy link
Copy Markdown
Contributor Author

Removed visual options (Enable by default) to avoid the user get confuse and make the search button more visual. What do you think @pilhuhn ? I think that this is clearer now
screenshot from 2019-02-27 09-48-18

@aljesusg aljesusg force-pushed the k_2456 branch 2 times, most recently from f1df8f2 to 798d333 Compare February 27, 2019 09:11
@pilhuhn
Copy link
Copy Markdown
Contributor

pilhuhn commented Feb 27, 2019

@aljesusg Much better. I personally (but I don't speak for UxD) would even put the search button next to the input boxes and not "far away" in the corner.

@aljesusg aljesusg added the do not merge A PR is not ready to merge label Feb 27, 2019
Comment thread src/store/ConfigStore.ts
whitelist: ['authentication', 'namespaces', 'serverConfig'],
transforms: [namespacePersistFilter]
transforms: [namespacePersistFilter, ...persitingJaegerReducers]
};
Copy link
Copy Markdown
Contributor Author

@aljesusg aljesusg Feb 28, 2019

Choose a reason for hiding this comment

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

Now I persist only the data required @josejulio

@hhovsepy
Copy link
Copy Markdown
Contributor

Tested all the issues mentioned in the description.
Only problem is when kiali is in https and jeager-query is with http, the Traces tab is hidden.
screenshot from 2019-02-28 13-38-58

@aljesusg
Copy link
Copy Markdown
Contributor Author

Fixed !!! Thanks !!!

@aljesusg aljesusg force-pushed the k_2456 branch 2 times, most recently from 8205c41 to 2053cd4 Compare February 28, 2019 16:31
@aljesusg aljesusg changed the title KIALI-2456 KIALI-2459 KIALI-2456 KIALI-2459 KIALI-2466 KIALI-2465 KIALI-2450 Feb 28, 2019
@aljesusg
Copy link
Copy Markdown
Contributor Author

aljesusg commented Mar 1, 2019

I keep the same namespace selector, and now you can see all services of the namespace selected like in jaeger view.
peek 2019-03-01 11-28

@aljesusg aljesusg removed the do not merge A PR is not ready to merge label Mar 1, 2019
@jotak
Copy link
Copy Markdown
Contributor

jotak commented Mar 1, 2019

Waw that's a title! :-) Maybe you could set a more meaningful title to better identify this PR in our notifications?

@aljesusg
Copy link
Copy Markdown
Contributor Author

aljesusg commented Mar 1, 2019

Waw that's a title! :-) Maybe you could set a more meaningful title to better identify this PR in our notifications?

So Jiras in the same PR ^^ I'll change to another title with more sense, In the description you can see the JIRAs related. Sorry @jotak ^^

@aljesusg aljesusg changed the title KIALI-2456 KIALI-2459 KIALI-2466 KIALI-2465 KIALI-2450 KIALI-2456,2459,2466,2465 and 2450 . Fix Bugs, improve toolbar for Jaeger Integration and refactor Mar 1, 2019
@hhovsepy
Copy link
Copy Markdown
Contributor

hhovsepy commented Mar 1, 2019

Verified all issues mentioned:
Toolbar, services, search button, namespaces, and redirecting when http.

screenshot from 2019-03-01 13-24-31
screenshot from 2019-03-01 13-26-15
screenshot from 2019-03-01 13-29-37

@lucasponce
Copy link
Copy Markdown
Contributor

One thing I see, that can be addressed in a different PR, is that traces tab is not linkable.
Parameters are not added into the url.

@lucasponce
Copy link
Copy Markdown
Contributor

lucasponce commented Mar 1, 2019

Sometimes the search button doesn't honor the parameters:
image

Same query:
image

That is confusing.

Copy link
Copy Markdown
Contributor

@lucasponce lucasponce left a comment

Choose a reason for hiding this comment

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

Commented with @aljesusg that there is a pending bug
#1025 (comment)
that probably is good to fix on this PR.

Other comments probably can be in separate tasks.

@aljesusg aljesusg changed the title KIALI-2456,2459,2466,2465 and 2450 . Fix Bugs, improve toolbar for Jaeger Integration and refactor KIALI-2456,2459,2466,2465, 2450 and 2507 . Improve toolbar for Jaeger Integration and refactor Mar 11, 2019
@aljesusg
Copy link
Copy Markdown
Contributor Author

Pr rdy for review @lucasponce @xeviknal @hhovsepy

@aljesusg aljesusg force-pushed the k_2456 branch 2 times, most recently from 59d111f to 21909a9 Compare March 11, 2019 11:25
@hhovsepy
Copy link
Copy Markdown
Contributor

Login page is not opened, it shows empty page, on both browsers.
Screenshot from 2019-03-11 12-48-19

@aljesusg
Copy link
Copy Markdown
Contributor Author

Login page is not opened, it shows empty page, on both browsers.
Screenshot from 2019-03-11 12-48-19

!!! What ??? No console output ? I didn't change nothing from login ^^ I am building the UI to check this

@aljesusg
Copy link
Copy Markdown
Contributor Author

Ok I found the problem, I had persistStore and I didn't see that. Thanks @hhovsepy

@aljesusg aljesusg force-pushed the k_2456 branch 2 times, most recently from 22a3368 to f745ffe Compare March 11, 2019 12:19
@aljesusg
Copy link
Copy Markdown
Contributor Author

Fixed @hhovsepy and I did the JIRA https://issues.jboss.org/browse/KIALI-2515 too, it was related with the status too.

Thanks !!!

@hhovsepy
Copy link
Copy Markdown
Contributor

Tested all the issues mentioned in the description.
Only problem is when kiali is in https and jeager-query is with http, the Traces tab is not redirecting.
Uploading Screenshot from 2019-03-11 14-05-41.png…

@hhovsepy
Copy link
Copy Markdown
Contributor

Screenshot from 2019-03-11 14-05-41

@aljesusg aljesusg force-pushed the k_2456 branch 2 times, most recently from d17eb8d to 2871fd6 Compare March 11, 2019 13:56
@hhovsepy
Copy link
Copy Markdown
Contributor

All verified now.

KIALI-2459 Fix traces tab if value is -1
KIALI-2466 Remove visual options in jaeger toolbar (Hide graph, hide summary) and make the search button bigger
KIALI-2465 Get Services in jaegerToolbar is not working after Openshift oauth changes
KIALI-2450 Change the namespace Selector in Jaeger Distributed tracing view to be like other views
KIALI-2507 Distributed tracing link disappear in a new tab
KIALI-2515 UI About - Components not persist when the user open a new tab
@lucasponce
Copy link
Copy Markdown
Contributor

Not a blocker for this PR, but I think we need to document on this Sprint all the steps/troubleshooting related with certificates, like this case:

image

Known issues, or something that is not really a Kiali issue but we can point users when they ask for it.

Copy link
Copy Markdown
Contributor

@lucasponce lucasponce left a comment

Choose a reason for hiding this comment

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

Testing locally and tested both with https + http to validate that links are external and https + https, where after accepting the certificate, everything seems to work as expected.

@lucasponce lucasponce merged commit 79edc7f into kiali:master Mar 12, 2019
@aljesusg aljesusg deleted the k_2456 branch September 9, 2019 08:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants