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

Display scaled time interval for Date Histogram when scaling is forced #12816

Closed
wants to merge 6 commits into from

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Jul 12, 2017

fixes #8618

The current behavior of Kibana is to display the requested time interval. This produces very confusing results when the time interval is scaled.

Before

In the screen shot below, notice how everything says time per second but the actual interval is day.

screen shot 2017-07-12 at 3 53 00 pm

After

With this PR in effect, the labels reflect the actual interval and are consistent with the underlying data.
screen shot 2017-07-12 at 4 14 36 pm

@nreese nreese added release_note:fix v6.0.0-beta1 Feature:Visualizations Generic visualization features (in case no more specific feature label is available) labels Jul 12, 2017
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

this is actually wrong and the behavior before was correct (but i agree its confusing, we got sooo many reports from users thinking this is wrong, also users asking us to implement the functionality as it is now, as they didn't realize it behaves this way).

if you look at your y-axis in the first screenshot, you will see that your counts are in the range of 0.008 ... how can this be ?

the thing is that you requested per second data. as you are looking at a big time range that got scaled and we asked elasticsearch for hourly data, but then we divided that value by 3600, to get second data.

so the chart title is correct, its displaying your data per second.

this is the right behaviour in my opinion. if i am plotting for example average visitors per hour, i don't care that i zoomed out to see the whole year, i am still interested in average visitors per hour per for december for example.

however if i am interested in absolute number of visitors i will the range to auto. this way i will always see absolute values in my chart and X axis title will get reflected as well.

@ppisljar
Copy link
Member

we should make it clearer what exactly happens .... so the tooltip on ! next to your period when scaling happens should describe this, as well as hovering on y-axis title should give you some indication of whats happening.

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

:) We are having this discussion a lot. It comes up regularly. In hindsight, the current approach (ie. "no resolution"), is not the best.

The problem is with the X-axis that the label isn't really correct, in either situation. The bucket size for example, WOULD be, per 5 minutes, or whatever the bucket-width is. But we rescale the readout on the Y-axis.

Reconsidering this, I think this suggestion from @LeeDr
#8618 (comment) is quite good. We need to label the Y-AXIS correctly, because that is where the scaling actually happens.

@nreese
Copy link
Contributor Author

nreese commented Jul 18, 2017

@thomasneirynck @LeeDr Does it make sense to label the x-axis as just the field name or the field name with the size of the bars? Would having two different time intervals be confusing (assuming the scaled interval is displayed on the y-axis as discussed above)?

@thomasneirynck
Copy link
Contributor

I'd prefer just labeling with the field-name (not the bucket-width), IF the y-axis label indicated the scaled value (e.g. average bytes per 5 seconds). That would reduce some of the confusion. Bucket-width isn't all that relevant imho.

@nreese
Copy link
Contributor Author

nreese commented Jul 18, 2017

Should the tooltip also contain the time interval?

screen shot 2017-07-18 at 9 24 10 am

@trevan
Copy link
Contributor

trevan commented Jul 18, 2017

I think that axis title is even more confusing now. "Count per 3 hours" is incorrect. The value is actually "Count per 3 hours/10800". Maybe "Count per 3 hours scaled down to second"?

@ppisljar
Copy link
Member

just Count per second ?

@nreese
Copy link
Contributor Author

nreese commented Jul 18, 2017

@trevan Thanks for speaking up. "Count per 3 hours" is not accurate.

I am fine with either suggestion from @trevon or @ppisljar. What would be preferred?

@nreese nreese force-pushed the date_histogram_scaled branch from 2c1cfb2 to 7b60d8d Compare July 18, 2017 18:38
@trevan
Copy link
Contributor

trevan commented Jul 18, 2017

@nreese, since this label only shows up if scaling is turned on and that only happens for "count" or "sum" metrics, I think the label should mention something about "scaling". It might be odd that if you switch to a "max" metric, the label goes from "Count per second" to just "Max". If it, instead, said "Count scaled to second", then the change to "Max" doesn't seem to look all that weird (to me at least).

And just to be clear, custom labels will override this, correct?

@trevan
Copy link
Contributor

trevan commented Jul 18, 2017

Oh, I just noticed you are getting rid of the "per timespan" from the x-axis if scaling is turned on. I don't like that idea. Especially since changing the metric will change the x-axis label. In your example, I'd probably go with something like "Count scaled to seconds" in y-axis and "@timestamp per 3 hours" in x-axis. Then the x-axis label doesn't change as you switch between metrics and the y-axis label indicates that a scaling took place.

@LeeDr
Copy link

LeeDr commented Jul 18, 2017

When you look at the tooltip in that screenshot where it shows Count 0.066
I think that is really Average Count per second
or Average Count per second in 3 hour buckets

The X-Axis scale really is just @timestamp. But I can see the case for indicating the 3 hour buckets as part of the X-Axis like it used to be @timestamp per 3 hours.

We need the 3 hours somewhere. And we need the per second also.

@LeeDr
Copy link

LeeDr commented Jul 19, 2017

Here's the test failure. I'm not sure if it's related to this change or not;

 1327 passing (1m)
  5 pending
  1 failing

  1) es/healthCheck/ensureTypesExist() v5 index adds types that are not in index:
     expected spy to be called 6 times but was called 5 times
    spy(indices.get, { feature: "_mappings", index: "ko" }) => [Promise] {  } at /var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-intake/src/core_plugins/elasticsearch/lib/ensure_types_exist.js:15:23
    spy(indices.putMapping, { body: { type: "text" }, index: "ko", type: "ral" }) => [Promise] {  } at /var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-intake/src/core_plugins/elasticsearch/lib/ensure_types_exist.js:61:13
    spy(indices.putMapping, { body: { type: "boolean" }, index: "ko", type: "udezihcev" }) => [Promise] {  } at /var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-intake/src/core_plugins/elasticsearch/lib/ensure_types_exist.js:61:13
    spy(indices.putMapping, { body: { type: "keyword" }, index: "ko", type: "fevziwut" }) => [Promise] {  } at /var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-intake/src/core_plugins/elasticsearch/lib/ensure_types_exist.js:61:13
    spy(indices.putMapping, { body: { type: "text" }, index: "ko", type: "roksucala" }) => [Promise] {  } at /var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-intake/src/core_plugins/elasticsearch/lib/ensure_types_exist.js:61:13
  AssertError: expected spy to be called 6 times but was called 5 times
      spy(indices.get, { feature: "_mappings", index: "ko" }) => [Promise] {  } at src/core_plugins/elasticsearch/lib/ensure_types_exist.js:15:23
      spy(indices.putMapping, { body: { type: "text" }, index: "ko", type: "ral" }) => [Promise] {  } at src/core_plugins/elasticsearch/lib/ensure_types_exist.js:61:13
      spy(indices.putMapping, { body: { type: "boolean" }, index: "ko", type: "udezihcev" }) => [Promise] {  } at src/core_plugins/elasticsearch/lib/ensure_types_exist.js:61:13
      spy(indices.putMapping, { body: { type: "keyword" }, index: "ko", type: "fevziwut" }) => [Promise] {  } at src/core_plugins/elasticsearch/lib/ensure_types_exist.js:61:13
      spy(indices.putMapping, { body: { type: "text" }, index: "ko", type: "roksucala" }) => [Promise] {  } at src/core_plugins/elasticsearch/lib/ensure_types_exist.js:61:13
      at Object.fail (node_modules/sinon/lib/sinon/assert.js:90:29)
      at failAssertion (node_modules/sinon/lib/sinon/assert.js:51:24)
      at Object.assertCallCount [as callCount] (node_modules/sinon/lib/sinon/assert.js:131:21)
      at Context.<anonymous> (src/core_plugins/elasticsearch/lib/__tests__/ensure_types_exist.js:114:20)
      at undefined.next (native)
      at step (src/core_plugins/elasticsearch/lib/__tests__/ensure_types_exist.js:6:1)
      at src/core_plugins/elasticsearch/lib/__tests__/ensure_types_exist.js:6:1



Warning: Task "simplemocha:all" failed.� Use --force to continue.

@lukasolson
Copy link
Member

@trevan Could you move that comment to this issue? #12692

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:fix stalled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Date Histogram on Bar chart does not update time interval in legend if scaling is forced
7 participants