-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Discover interval #3290
Discover interval #3290
Conversation
There is still some auto-scaling happening here, for example: I'm not sure it this is something that will be adjust in #2991, but my guess is that it's really not something we can fix. Showing the value that's used might be helpful though. |
I think the auto scaling is OK here, Feb 6 - March 6 as an hourly chart would just be too many bars. I agree it would be extremely helpful to indicate that we are not showing raw counts at this point, but rather an adjusted "rate" |
@rashidkpc Would you like me to add the current interval to this PR as well then? |
Probably worth talking about, I'm not sure where it belongs exactly? Seems like it should go on the X-axis label somehow. Thoughts? |
Whoops, didn't see the question. I could display it on the axis, maybe as part of the label. Let me play with this a bit and see if I can come up with something that looks decent. |
Conflicts: src/kibana/plugins/discover/controllers/discover.js src/kibana/plugins/discover/index.html
@rashidkpc I opened #3535 as displaying the interval is becoming a little more involved, and I don't think it should block merging this feature. Sending back to you for review. |
Found an issue - when you save the search, you lose the interval state. Need to fix that. |
The issue right now is that the interval is only saved in the app state, which means when you save a vis, you lose the interval (since it's reloaded, and the app state is discarded). I'm leaving this somewhat broken right now, with plans to fix it via #2716 by making that solution handle arbitrary persisted states, not limited to just visualizations. |
I think its ok to have the interval as transient for now. The chart is more of a navigational aid in the context so I don't see it as something essential to put on the savedSearch. |
This is what I ended up with. I reset the display when the user changes the interval back to Auto I'm using the bucket interval description (which dips in to #3535 territory) as it's the best label we have available. Alternately, I could add new labels tot he interval options, or perhaps change what's there. If that's more desirable, let me know. |
|
||
<span class="results-interval" ng-hide="showInterval"> | ||
<a | ||
ng-click="$parent.showInterval = !$parent.showInterval"> |
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.
Accessing $parent to set the value from the view seems wrong. Maybe change this to be a function on the parent that sets the value. Eg:
$scope.toggleInterval = function () {
$scope.showInterval = !$scope.showInterval;
};
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.
That's how I started doing this, but it wasn't working for some reason, and $parent seemed to fix it. Of course, now that I do it again, it works fine. Oi.
With that one comment this looks good. Would like another set of eyes here. |
Besides that one comment, this LGTM! |
Ready for one last look |
Closes #3038
Adds interval selection to Discover, using the same interval logic used in the agg builder in visualize.
To add addition intervals (mostly, intervals that are not just 1), see #1784
description
)