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

React UI: Select time range with mouse drag #8977

Merged
merged 4 commits into from
Jun 30, 2021

Conversation

LeviHarrison
Copy link
Member

This PR adds support to select the time range with a mouse drag.

time-select.mp4

Fixes #1237

Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
Comment on lines 3 to 8
* THIS FILE WAS COPIED INTO PROMETHEUS FROM GRAFANA'S VENDORED FORK OF FLOT
* (LIVING AT https://github.com/grafana/grafana/tree/v7.5.8/public/vendor/flot),
* WHICH CONTAINS FIXES FOR DISPLAYING NULL VALUES IN STACKED GRAPHS. THE ORIGINAL
* FLOT CODE WAS LICENSED UNDER THE MIT LICENSE AS STATED BELOW. ADDITIONAL
* CHANGES HAVE BEEN CONTRIBUTED TO THE GRAFANA FORK UNDER AN APACHE 2 LICENSE, SEE
* https://github.com/grafana/grafana/blob/v7.5.8/LICENSE.
Copy link
Member Author

Choose a reason for hiding this comment

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

I added the v7.5.8 distinction as that was the last version that was Apache 2 licensed and the one I got this file from.

Copy link
Member Author

Choose a reason for hiding this comment

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

I copied the header from another file and I just noticed it says CONTAINS FIXES FOR DISPLAYING NULL VALUES IN STACKED GRAPHS, which has nothing to do with this plugin, but I checked and all the other plugins have it. Maybe we should change that?

Copy link
Member

Choose a reason for hiding this comment

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

We should update the license in other files

Copy link
Member Author

Choose a reason for hiding this comment

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

@juliusv Do you know which one this line actually pertains to?

Copy link
Member

Choose a reason for hiding this comment

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

Nope! So #6603 just copied Grafana's vendored flot version into Prometheus. The reason for this was that the normal Flot version didn't handle null values in stacked graphs correctly, but Grafana's fork did, because they did something to it to fix it. So I guess one would have to do some archaeological digging in Grafana's Flot fork itself to try and find out what they changed, but not sure how easy it is. Also, who knows, maybe upstream Flot has been fixed wrt. stacked null values in the meantime.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll remove that line from this new file because we definitely know it has nothing to do with the fix. Doing updates on the other files/maybe using the new version of upstream Flot is a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

It may also be good to add the v7.5.8 tag to the other links, especially the ones referring to the Apache 2 license that is now not an Apache 2 license.

@roidelapluie
Copy link
Member

Would it be relevant to unzoom on double click ? (unzoom with the double-clicked point as center)?

@roidelapluie
Copy link
Member

Note: This is awesome :)

@juliusv
Copy link
Member

juliusv commented Jun 21, 2021

Would it be relevant to unzoom on double click ? (unzoom with the double-clicked point as center)?

So far I am happy with just pressing the "back" button on my mouse, but not sure. Is double click intuitive for unzooming?

@LeviHarrison
Copy link
Member Author

I think that double-click is too subtle an action. I'm sure many accidentally double-click on a regular basis, especially with a click-heavy element such as the graph.

Signed-off-by: Levi Harrison <git@leviharrison.dev>
@RichiH
Copy link
Member

RichiH commented Jun 22, 2021

Nice feature!

Adding /* SPDX-License-Identifier: Apache-2.0 */ to jquery.flot.selection.js and noting where you copied the file from might be the best course of action.

I have adding SPDX Ids on my bucket list; this would should make licence checking easier going forward. Unclear if I will find time to apply them all to the old branches as well.

@roidelapluie
Copy link
Member

Nice feature!

Adding /* SPDX-License-Identifier: Apache-2.0 */ to jquery.flot.selection.js and noting where you copied the file from might be the best course of action.

I have adding SPDX Ids on my bucket list; this would should make licence checking easier going forward. Unclear if I will find time to apply them all to the old branches as well.

Instead of doing it yourself, why not creating an issue ? The community might help here. That's a lot of repos.

@RichiH
Copy link
Member

RichiH commented Jun 22, 2021

I fear that outside of you and me, most people wouldn't care. The community interest is so low that I needed to start https://github.com/RichiH/spdx_helper myself as there was nothing to automate addition (it needs to detect and exclude vendored files before I can make the first production runs).

@LeviHarrison
Copy link
Member Author

If there's a consensus, I'd be happy to start the process here. Just to be clear, it would look something like this:

/*
SPDX-License-Identifier: Apache-2
Source: https://github.com/grafana/grafana/tree/v7.5.8/public/vendor/flot/jquery.flot.selection.js
*/

@roidelapluie
Copy link
Member

We still need the apache header:

https://www.fosslife.org/how-apply-license-your-open-source-project

@LeviHarrison
Copy link
Member Author

/*
Copyright 2015 Grafana Labs
SPDX-License-Identifier: Apache-2
Source: https://github.com/grafana/grafana/tree/v7.5.8/public/vendor/flot/jquery.flot.selection.js

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

    http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

Ok here's a revised version. I garnered the copyright directly from the license. It is worth mentioning that I had to make some small modifications to get it to work, I'm not sure if that changes anything with the license stuff.

@RichiH
Copy link
Member

RichiH commented Jun 22, 2021

@roidelapluie I disagree with pulling the full boilerplate in. But we're also at risk of bikeshedding

@LeviHarrison Please bear with us while we figure this out. It's not fair to you that need to follow our discussion

@roidelapluie
Copy link
Member

@roidelapluie I disagree with pulling the full boilerplate in. But we're also at risk of bikeshedding

VM is way more competent than me in this area, I trust her.

@RichiH
Copy link
Member

RichiH commented Jun 22, 2021

It's somewhat hard to argue with "someone else is right"; more of a monologue than a conversation.

The problem I see is that we would be saving literally the same information twice; the inherent risk is that this can diverge over time. A license blurb is also more legally binding than an SPDX header, as such the header becomes useless and you need to parse the file with brittle heuristics again. The law systems I am familiar with (Germany, USA) both rely on intent. Putting a SPDX header in makes the intent clear. As a court would always try to find out and follow intent, the SPDX header itself should(TM) be enough.

Along similar lines, claiming that Grafana Labs holds all the copyright seems wrong; https://github.com/grafana/grafana/blob/v7.5.8/public/vendor/flot/jquery.flot.selection.js clearly states that initial copyright was "Copyright (c) 2007-2013 IOLA and Ole Laursen." Having done this sleuthwork helping Debian's ftp-master ages ago, I know how endless this digging can be. A relative pointer to where the file was copied from initially seems to be enough for someone to dig if they needed to dig.

All that being said, I don't think this discussion should block the merge. For the reasons stated above I strongly prefer the short form proposed in #8977 (comment) but don't want to block either.

@RichiH
Copy link
Member

RichiH commented Jun 22, 2021

Deliberately in a second comment: "Back" makes more sense to me than a double-click. In more complex drill-downs it can become unclear what the double-click would jump back to whereas atomic back steps would be explicit and predictable.

@roidelapluie
Copy link
Member

double click is not the same as back, double click is zoom out. this is what grafana is doing.

@roidelapluie
Copy link
Member

e.g. you do not need to zoom first to double click, you can do it directly.

@LeviHarrison
Copy link
Member Author

I believe zoom out is range control, equivalent to entering a time range into the box on the graph controls, is it not? If so, I think it's unrelated to this and should be considered as a separate feature.

Copy link
Member

@roidelapluie roidelapluie left a comment

Choose a reason for hiding this comment

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

Let's unblock this

@juliusv
Copy link
Member

juliusv commented Jun 30, 2021

👍

@juliusv juliusv merged commit 167dfa1 into prometheus:main Jun 30, 2021
@LeviHarrison LeviHarrison deleted the graph-select-time-range branch August 7, 2021 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expression graphs: Allow time selection with mouse drag
4 participants