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

ui: Do not graph series which have no data points #22626

Merged
merged 1 commit into from Feb 14, 2018

Conversation

mrtracy
Copy link
Contributor

@mrtracy mrtracy commented Feb 13, 2018

Graph lines which have no data points in the queried time span will no
longer be displayed at all; they will not appear in the legend or the
interactive tooltip. This situation has proved a consistent source of
confusion, especially on graphs which display (empty) values for
decommissioned nodes which are no longer producing data.

Fixes #19541

Release note: Fixed an issue where graph tooltips would continue to
display zero values for nodes which had long been decommissioned.

Graph lines which have no data points in the queried time span will no
longer be displayed *at all*; they will not appear in the legend or the
interactive tooltip. This situation has proved a consistent source of
confusion, especially on graphs which display (empty) values for
decommissioned nodes which are no longer producing data.

Fixes cockroachdb#19541

Release note: Fixed an issue where graph tooltips would continue to
display zero values for nodes which had long been decommissioned.
@mrtracy mrtracy requested a review from a team as a code owner February 13, 2018 05:26
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@couchand
Copy link
Contributor

code LGTM. My only question is if we want to check that the node is decommissioned first, but I guess perhaps that would be better to roll into the wider question of how to show "no data" points for non-decomissioned nodes.

This still doesn't close the door on UX for decomissioned nodes, for instance #19782 was supposed to be related to decomissioning, and I wouldn't be surprised if other surprises crop up, but I suppose this makes progress.


Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@mrtracy
Copy link
Contributor Author

mrtracy commented Feb 13, 2018

This isn't meant to close the door on decommissioned nodes, the issue it is resolving is very specific (nodes polluting graph tooltips); this is an issue that users are encountering and complaining about.

The "corollary", if you will - a node which is live according to other metrics, but is not recording any time series when expected - doesn't seem to be a likely enough scenario that we need to tackle it in the same swipe.

@mrtracy
Copy link
Contributor Author

mrtracy commented Feb 14, 2018

After discussing this some more with @couchand, we agree that it's better to merge this than not. We think that there might be some cases where the absence of data for a line needs to be conspicuous, but they are outweighed by the annoying cases where it should be hidden, and will ultimately be better addressed by events-on-graphs or a similar feature.

@mrtracy mrtracy merged commit 9cefac0 into cockroachdb:master Feb 14, 2018
@mrtracy mrtracy deleted the mtracy/ui_graphs_no_data branch February 14, 2018 19:51
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.

None yet

3 participants