-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
UI: rework graph page #2150
UI: rework graph page #2150
Conversation
We've fixed UI a bit in our fork, because UI getting stuck always right after opening. For us, this bug is a huge pain, so we decided to fix it and share. I'm not sure that it's legal to make such changes in Thanos without making it in Prometheus UI first, just curious what do you think @bwplotka ? |
I believe this is yet another attempt at solving prometheus/prometheus#5421 and prometheus/prometheus#5919. We should really probably do this or something similar. It seems like this prometheus/prometheus#6100 upstream never got any attention. Maybe @bwplotka has some new information regarding it? If not, then we should probably really finally do this ourselves, half a year after. However, because there are different implementations which are aiming to solve this problem, we should probably start with a design doc which would weigh pros/cons. Thoughts? |
Yes, let's do this. The conclusion upstream was: "Let's fix this in
ReactUI"
Happy to fix this in both React and old one for Thanos!
…On Tue, 18 Feb 2020 at 20:38, Giedrius Statkevičius < ***@***.***> wrote:
I believe this is yet another attempt at implementing
prometheus/prometheus#5421
<prometheus/prometheus#5421> and
prometheus/prometheus#5919
<prometheus/prometheus#5919>. We should really
probably do this or something similar. It seems like this
prometheus/prometheus#6100
<prometheus/prometheus#6100> upstream never got
any attention. Maybe @bwplotka <https://github.com/bwplotka> has some new
information regarding it? If not, then we should probably really finally do
this ourselves, half a year after.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2150?email_source=notifications&email_token=ABVA3O2MCXBT3TMXTPSYB63RDRBMZA5CNFSM4KXIU5KKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMET2GY#issuecomment-587808027>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABVA3O3PVSV2MCFYYYW4ZU3RDRBMZANCNFSM4KXIU5KA>
.
|
@GiedriusS regarding to prometheus/prometheus#6100 I can also fix initial page freezing in this PR. In upstream it fixed here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @d-ulyanov since Prometheus does not want to merge fix, we have not much choice (: It's LGTM from me.
Can we rebase?
@vankop please, rebase and let's merge it 🚀 |
5ceb97a
to
1f57fa9
Compare
Throttling user input, Add minimum query length == 3, Maximum amount of suggestions to show from 1000 to 20/30, improve performance of fuzzy matching a lot (less allocations, run matching on previous results if possible) Signed-off-by: Ivan Kopeykin <[email protected]>
1f57fa9
to
6e0409e
Compare
This issue/PR has been automatically marked as stale because it has not had recent activity. Please comment on status otherwise the issue will be closed in a week. Thank you for your contributions. |
Still valid. Seems like there's a conflict :/ |
Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Signed-off-by: Ivan Kopeykin [email protected]
Changes
Verification
Tested in our environment with more than 500_000 metrics
/cc @d-ulyanov