Navigation Menu

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

ReactUI: adopt grafana flot fix for stacked graphs #6603

Merged
merged 9 commits into from Jan 14, 2020

Conversation

boyskila
Copy link
Contributor

Fixes: #6576

@boyskila boyskila force-pushed the vendoring-flot branch 3 times, most recently from 7d303dc to 84dc0ce Compare January 11, 2020 23:08
@juliusv
Copy link
Member

juliusv commented Jan 11, 2020

The behavior looks good to me, thanks! @roidelapluie could you also double-check at https://deploy-preview-6603--prometheus-react.netlify.com/new/graph?g0.expr=label_replace(vector(time())%20%25%20300%20%3C%20180%2C%22x%22%2C%22x%22%2C%22%22%2C%22%22)%20or%20label_replace(vector(time())%20%25%20300%20%3E%20140%2C%22y%22%2C%22y%22%2C%22%22%2C%22%22)&g0.tab=0&g0.stacked=1&g0.range_input=1h

I think it'd be good to change the PR / commit description to note that we are vendoring Grafana's Flot version, not just adopting a single fix from it (although that is the purpose, which should still be noted).

We also need to do our best regarding copyright notices, since Grafana did some changes on its Flot version after forking it, but didn't add new copyright headers :( @davkal as my go-to Grafana person, what would you advise us to do here?

@roidelapluie
Copy link
Member

+1 on everything.

@davkal
Copy link
Contributor

davkal commented Jan 14, 2020

In my layman's opinion: We didnt rewrite flot, so I think the original copyright notice holds. For maintainability I suggest you add a comment linking to the vendored version in Grafana.

Also I'm pretty sure stacking is broken if you have timeseries with missing datapoints.

@juliusv
Copy link
Member

juliusv commented Jan 14, 2020

@davkal Grafana does have a bunch of local changes to the files in https://github.com/grafana/grafana/tree/master/public/vendor/flot, so while the original notices should be preserved, it seems that additional ones should be added as well. @boyskila How about also adding a note at the top of each copied file:

This file was copied into Prometheus from Grafana's vendored fork of Flot (living at https://github.com/grafana/grafana/tree/master/public/vendor/flot), which contains fixes for displaying null values in stacked graphs. The original Flot code was licensed under the MIT license as reproduced below. Additional changes have been contributed to the Grafana fork under an Apache 2 license, see https://github.com/grafana/grafana/blob/master/LICENSE.

I'm not a lawyer either, but Grafana people are friends of Prometheus, and this is hopefully a good-enough effort to show what came from where.

@davkal We actually tested stacking with your vendored fork, and at least as far as we could see (see Prometheus link above), it fixes the null stacking bug that is present in upstream Flot.

@boyskila
Copy link
Contributor Author

Ok, sounds good. Hope tomorrow someone's lawyer not going to knocking on my door :)

Signed-off-by: blalov <boiskila@gmail.com>
Signed-off-by: Boyko Lalov <boiskila@gmail.com>
Signed-off-by: blalov <boiskila@gmail.com>
Signed-off-by: Boyko Lalov <boiskila@gmail.com>
Signed-off-by: blalov <boiskila@gmail.com>
Signed-off-by: Boyko Lalov <boiskila@gmail.com>
Signed-off-by: blalov <boiskila@gmail.com>
Signed-off-by: Boyko Lalov <boiskila@gmail.com>
Signed-off-by: blalov <boiskila@gmail.com>
Signed-off-by: Boyko Lalov <boiskila@gmail.com>
…flag

Signed-off-by: blalov <boiskila@gmail.com>
Signed-off-by: Boyko Lalov <boiskila@gmail.com>
Signed-off-by: Boyko Lalov <boiskila@gmail.com>
Signed-off-by: Boyko Lalov <boiskila@gmail.com>
* THIS FILE WAS COPIED INTO PROMETHEUS FROM GRAFANA'S VENDORED FORK OF FLOT
* (LIVING AT HTTPS://GITHUB.COM/GRAFANA/GRAFANA/TREE/MASTER/PUBLIC/VENDOR/FLOT),
* WHICH CONTAINS FIXES FOR DISPLAYING NULL VALUES IN STACKED GRAPHS. THE ORIGINAL
* FLOT CODE WAS LICENSED UNDER THE MIT LICENSE AS REPRODUCED BELOW. ADDITIONAL
Copy link
Member

Choose a reason for hiding this comment

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

I'd put this new notice header at the very top. Then the word "BELOW" also makes sense :)

Not sure why it'd all be capitalized? At least the URLs are broken through the capitalization now.

Copy link
Member

Choose a reason for hiding this comment

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

And: actually my "as reproduced below" wasn't correct, since the original files don't contain a full reproduction of the MIT license. So "as reproduced below" -> "as stated below".

Signed-off-by: Boyko Lalov <boiskila@gmail.com>
@juliusv
Copy link
Member

juliusv commented Jan 14, 2020

👍 Thanks!

@juliusv juliusv merged commit e12e5ec into prometheus:master Jan 14, 2020
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.

React UI: Stack graph displays null value as zero
4 participants