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

Logs are effectively impossible to copy when the log window has scroll-bar #1508

Closed
Ark-kun opened this issue Jun 14, 2019 · 15 comments · Fixed by #2370
Closed

Logs are effectively impossible to copy when the log window has scroll-bar #1508

Ark-kun opened this issue Jun 14, 2019 · 15 comments · Fixed by #2370

Comments

@Ark-kun
Copy link
Contributor

Ark-kun commented Jun 14, 2019

There is something wrong with log line selection. You cannot select more lines than fits in the window and when you scroll, some weird things happens with the selection.
This is probably caused by the line numbering UX component.
I'd prefer working selection over line numbering (whih might not add much value to the logs).

@Bobgy
Copy link
Contributor

Bobgy commented Sep 9, 2019

/assign @Bobgy

@Bobgy
Copy link
Contributor

Bobgy commented Sep 9, 2019

@Ark-kun Do you mean selecting log of a pipeline run step? I tried this and it selects with no issues when log has more than one screen with chrome on a macbook.

Is this issue already fixed? or it only reproduces on a different device?

@Bobgy
Copy link
Contributor

Bobgy commented Sep 9, 2019

My attempt video: https://drive.google.com/file/d/1flmz7jiM84nZ8DgPTBMNiM-T8oUJWEVq/view
(I didn't use a new release, but it is bundled with KF 0.6.1 released on 2019/6/27. I think it should cover this issue.

@martin-laurent
Copy link

I'm not sure this video is testing the right thing. For example, given your video, the faulty behavior is that, if you were to copy/paste the text selected you would get only lines from 12 to 41 instead of 7 to 41 as you would expect. This is terrible for long logs.

@mattnworb
Copy link
Contributor

The problem is especially pronounced (in my experience) when the component is still running and therefore producing new logs.

@Bobgy
Copy link
Contributor

Bobgy commented Sep 9, 2019

Thanks for the explanation. Now I understand the problem. Will think about a fix.

@Bobgy
Copy link
Contributor

Bobgy commented Oct 10, 2019

Sorry for not replying for a while, because I've got some other stuff on my priority list. I just tried to investigate the issue today.

The problem is with our own implementation of LogViewer component. It uses react-virtualized under the hood to deal with large size of logs. react-virtualized only renders visible and a few more hidden lines on the screen. Therefore, it doesn't work with selection + scrolling well.

If selection is an important feature, then my proposed workaround is to only render like last 1000 lines of log in UI and stop using react-virtualized.

Reasoning:

  • Selection works natively without virtualization
  • UI won't crash when log size is huge. It just provides limited content (last 1000 lines), which I think is enough. If more is needed, user can choose another alternative to view logs.

@Bobgy
Copy link
Contributor

Bobgy commented Oct 10, 2019

@Ark-kun what do you think about the solution?

@Bobgy
Copy link
Contributor

Bobgy commented Oct 10, 2019

@Bobgy
Copy link
Contributor

Bobgy commented Oct 10, 2019

I also researched npm libraries that we can use out of box.

I found this one, but I worry the amount of maintenance it gets: https://github.com/mozilla-frontend-infra/react-lazylog

@martin-laurent
Copy link

Personally I think that keeping only 1000 lines of code would NOT be a good idea as that would hide logs of things happening at startup (before a really long tensorflow stacktrace for example).

If anything, there is a related bug where there is either links to stackdriver logs OR the logs in the UI but never both. Having them both always visible (or at least the stackdriver logs) always visible would be better.

Also when the component is running (before it is fully done) the it's impossible to scroll, because the viewing component seems to force the position at the bottom y every few seconds which is really annoying.

If these two things can be fixed, personally I'd be in a much happier place.

@Bobgy
Copy link
Contributor

Bobgy commented Oct 11, 2019

@martin-laurent thanks for the valuable feedback.

What about top 1000 lines + bottom 1000 lines? that will be trickier to implement, but still doable.
EDIT: I found a better solution. Do you think you would ever want to select more than 1000 lines? Basically, I can configure UI to render 1000 extra lines both above and below the line you scrolled to, so as long as you don't need to select more than 1000 lines, this would work.
(if you need to copy all the log, a separate button should be provided IMO)
EDIT2: Here's a POC you can reference: https://drive.google.com/file/d/1jTg0mztuT0_Vq2lkRlQnjS0i8PEP3vdo/view

If anything, there is a related bug where there is either links to stackdriver logs OR the logs in the UI but never both. Having them both always visible (or at least the stackdriver logs) always visible would be better.

Can you point me to the issue? I think that should be fixed for sure.

Also when the component is running (before it is fully done) the it's impossible to scroll, because the viewing component seems to force the position at the bottom y every few seconds which is really annoying.

This is our bug, I can easily fix it.

I want to change it to only scroll to bottom when it was already scrolled to bottom. If users scrolled up manually, then it will remain its position.

@Bobgy
Copy link
Contributor

Bobgy commented Oct 11, 2019

I made a PR: #2370 that can fix:

  • this issue, at most 1000 lines of logs can be copied when scrolling
  • Also when the component is running (before it is fully done) the it's impossible to scroll, because the viewing component seems to force the position at the bottom y every few seconds which is really annoying.

DEMO: https://drive.google.com/file/d/1jTg0mztuT0_Vq2lkRlQnjS0i8PEP3vdo/view

@martin-laurent Would this solve your problem?

@martin-laurent
Copy link

@Bobgy Looks perfect IMO

@Bobgy
Copy link
Contributor

Bobgy commented Oct 12, 2019

@martin-laurent Great, I will continue with the approach.

magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this issue Oct 22, 2023
* add native deployment

update log package

seprate native reconciler into deployment,service,ingress and native

add some debug log and use networking/v1 instead of networking/v1beta1

fix bug: set default value to deployment container spec not work

remove IngressDomain from required Ingress config list

add transformer raw deployment support

fix build error

fix raw deployment e2e test

follow the comments

add a check Container exist

fix create service crash bug

update e2e test

update raw deployment e2e test

add container check in create raw ingress

update raw deployment e2e test

pass the annotation to ingress spec(ingress.class), update the raw e2e test

add package for test

modify e2e test for raw deployment

fix python import package

fix python import package

fix e2e raw deployment error

fix pylint error

fix e2e test bug

fix e2e test bug

fix e2e test bug

fix e2e test bug

fix set default raw ingress domain

update raw deployment e2e test.

update raw deployment e2e test.

update raw deployment e2e test.

update raw deployment test

fix istio ingress return 404: remove path field

disable creating istio virtual service when set raw deployment is true

disable creating istio virtual service when set raw deployment is true

disable creating istio virtual service when set raw deployment is true

add virtual service

change the rules of raw inferenceservice

update e2e test

update e2e test

update e2e test

update e2e test

update e2e test

fix transformer can not access predictor bug

update raw-transformer test

update raw-transformer test

update raw-transformer test

remove unuseless args

modify log info

* Address comments

* Update ingress creation logic

* Add readiness probes

* Reconcile explainer

* Add test for raw kube reconciler

* Add raw deployment integration tests

Co-authored-by: Dan Sun <dsun20@bloomberg.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants