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

yaxis tick formatters #9065

Merged
merged 14 commits into from
Feb 10, 2017
Merged

yaxis tick formatters #9065

merged 14 commits into from
Feb 10, 2017

Conversation

ppisljar
Copy link
Member

Ability to specify one of (bits,bits/s,bytes,bytes/s,currency,custom) unit formatters for y-axis labels.

based on elastic/timelion#99 thanks and credits to @pagenbag

Example below of :

.label("series1:bandwidth").yaxis(units=bits/s,max=2000000)
.label("series2:currency").yaxis(2,units=currency:EUR,max=2000000)
.label("series3:custom").yaxis(3,position=right,units="custom:~ : things",max=2000000)

6f199076-0254-11e6-816c-70e7b1e03e63 1

@ppisljar ppisljar added Feature:Timelion Timelion app and visualization review labels Nov 14, 2016
@rashidkpc
Copy link
Contributor

I can't get the custom format to work at all, can you? I just get a blank chart

screen shot 2016-11-14 at 10 21 11 am

The currency formatting functionality is confusing. I can't get the example in the screenshot to work and the spec for toLocaleString says you need to know the 3 letter ISO 4217 currency code. Perhaps specifying the symbol, as in the screenshot, is a non-standard thing implemented in some other browser than Chrome?

@ppisljar
Copy link
Member Author

ppisljar commented Nov 14, 2016

for currency you need to provide 3 letter ISO code that's correct. i changed that from original implementation.

so to get euros units=currency:EUR ... or for dollars units=currency:USD

i fixed the custom format.

@rashidkpc
Copy link
Contributor

Should probably note in the help text for the units parameter that the currency requires the 3 letter ISO 4217 currency code. Or maybe just get rid of the currency options and just use 'custom:$'. I suspect that would be sufficient for most people?

@ppisljar
Copy link
Member Author

i like that currency also adds the commas on the thousands and limits decimal places to 2 ... i'll add the information to help text.

'bits/s': function (val, axis) {
var labels = ['b/s','kb/s','mb/s','gb/s','tb/s','pb/s'];
var index = 0;
while (val > 1000 && index < labels.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

val > 1000 should probably be val >= 1000 otherwise you end up with stuff like this:

screen shot 2016-11-14 at 12 45 52 pm

@rashidkpc
Copy link
Contributor

The bit above lead me to another issue. When using the bytes mode the ticks end up placed at unnatural points:

Currently:
screen shot 2016-11-14 at 1 12 31 pm

What I would expect:
screen shot 2016-11-14 at 1 13 35 pm

The logic for the 2nd chart can be cribbed from https://github.com/whatbox/jquery.flot.byte/blob/master/jquery.flot.byte.js

I wouldn't necessarily use that plugin, but it does the right thing.

@tbragin tbragin added the Feature:Visualizations Generic visualization features (in case no more specific feature label is available) label Nov 14, 2016
@ppisljar
Copy link
Member Author

i took the logic for generating byte ticks from the plugin you pointed me to. i kept just the basic part we need.

@pagenbag
Copy link

Yay - thanx @ppisljar :)

@rashidkpc
Copy link
Contributor

rashidkpc commented Nov 16, 2016

You need to take ['bits, bits/s'] out of the modes to pass to tickGenerator, otherwise you end up with stuff like this on the chart on the left.

screen shot 2016-11-16 at 10 02 46 am

What you want is this:
screen shot 2016-11-16 at 9 56 09 am

Also, as @simianhacker noted to me, you can get rid of all of the code in tick_formatters.js and replace it with calls to numeral.js which is already ships with Kibana. Here's how @simianhacker is doing tickFormatters: https://github.com/elastic/thor/blob/master/public/lib/tick_formatter.js

if (!tickFormatters[unitTokens[0]]) {
throw new Error (`${units} is not a supported unit type.`);
}
myAxis.units = unitTokens;
Copy link
Contributor

Choose a reason for hiding this comment

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

For the sake of API consistency and readability of the returned object, I'd rather see this returned as

{
  "type": "someType",
  "prefix": "foo",
  "suffix": "bar"
}

return base * Math.floor(n / base);
}

function generateTicks(axis) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Break this function out and write tests for it

@@ -218,6 +260,16 @@ module.exports = function timechartFn(Private, config, $rootScope, timefilter, $
// best you can do is an empty string. Deal with it.
if (objVal == null) return srcVal;
});

_.forEach(options.yaxes, yaxis => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks 2nd y-axis functionality

@ppisljar
Copy link
Member Author

i fixed most things you brought up but:

  • there is a bug with secondary y axes in timelion which i think we should address in separate PR
  • there's some discussion going on about depending on numeraljs for user entered information ... for now i would prefer not to depend on it.

@ppisljar
Copy link
Member Author

timelion doesnt have any tests for the browser part does it ?

@rashidkpc
Copy link
Contributor

rashidkpc commented Nov 17, 2016

  • we should fix that one first, it sounds like the 2nd y-axis is partially broken, we don't want it completely broken
  • Both of those tickets are about numeral.js as a user-facing API. This wouldn't directly expose numeral to the user, it would be an implementation detail, as numeral should be.

Timelion doesn't have tests for the browser stuff, but pulling out that function and adding tests for it shouldn't be much work. If you take it out of the file you can test it in isolation. There's a fair amount of logic there.

edit: I see you added tests, solid.

@ppisljar
Copy link
Member Author

ppisljar commented Nov 18, 2016

hey @rashidkpc yes tests are there, also i fixed the axis (its still partly broken due to that bug, but its not completely broken .... so this PR doesnt break it additionally, and once that bug is fixed it should work correctly).

with numeral.js i don't seem to be able to do bits, bits/s, bytes/s and currency (in a way i do it now, where you just submit ISO currency code and it will auto format to match ... with numeral you can just add a sign there (which is tricky for chinese for example ... where on my keyboard is CNY sign ? ) ) or am i just not getting something ?

@rashidkpc
Copy link
Contributor

rashidkpc commented Nov 18, 2016

This now seems to be broken completely unless I call at least one series with .yaxis(#). Rendering fails relatively silently, only logging a console message, but the chart isn't redrawn without .yaxis(#)

Also the currency formatter is currently broken. The tick_formatters.js file needs tests. You're referencing axis.options.prefix in the formatter, but its actually under axis.options.units.prefix

On a side note, its funny you mention the Chinese yuan, since its an excellent example of lack of prefix standardization in currency.

In China you would see ¥10.00, however the rest of the world uses '¥' to refer to the Japanese yen. The official name of the yuan is of course the renminbi, and you'll often see it abbreviated as RMB outside of China, eg RMB10.00. With this code we get Javascript's toLocaleString() output: CN¥10.00, which is a weird mashup, especially for web browsers. By default they treat ¥ as word break boundary and will wrap CN and ¥10.00 onto different lines.

In any case, ISO 4217 provides a standard for something, so it'll have to do.

@rashidkpc
Copy link
Contributor

Everything about this looks fine, with one exception. I'd like it to throw an error if the currency isn't either null (thus defaults to USD) or a 3 letter code, eg /[A-Za-z]{3}/, otherwise the user just gets a blank chart.

const threeLetterCode = /^[A-Za-z]{3}$/;
const currency = unitTokens[1];
if (currency && !threeLetterCode.test(currency)) {
throw new Error('Currency must be a three letter code');
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a test

@rashidkpc
Copy link
Contributor

Well this is weird...

Seems to only happen with the custom formatter and only when the data is all between 0 and 1. Passing .precision(1) doesn't seem to fix it either. I swear I'm not using an intel processor from the early 90s, so a buggy FPU is out of the question 😄

screen shot 2016-12-02 at 9 20 04 am

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.

I got the following error on the server-console, when specifying a non-existant unit type:

Unhandled rejection Error: time is not a supported unit type.
    at /home/thomas/repos/kibana/src/core_plugins/timelion/server/series_functions/yaxis.js:80:17
    at /home/thomas/repos/kibana/src/core_plugins/timelion/server/lib/alter.js:22:17
    at arrayMap (/home/thomas/repos/kibana/node_modules/lodash/index.js:1406:25)
    at map (/home/thomas/repos/kibana/node_modules/lodash/index.js:6710:14)
    at interceptor (/home/thomas/repos/kibana/node_modules/lodash/index.js:12240:26)
    at thru (/home/thomas/repos/kibana/node_modules/lodash/index.js:5927:26)
    at baseWrapperValue (/home/thomas/repos/kibana/node_modules/lodash/index.js:2768:30)
    at LazyWrapper.lazyValue [as value] (/home/thomas/repos/kibana/node_modules/lodash/index.js:1077:16)
    at baseWrapperValue (/home/thomas/repos/kibana/node_modules/lodash/index.js:2761:25)
    at LodashWrapper.wrapperValue (/home/thomas/repos/kibana/node_modules/lodash/index.js:6124:14)
    at /home/thomas/repos/kibana/src/core_plugins/timelion/server/lib/alter.js:23:18
    at tryCatcher (/home/thomas/repos/kibana/node_modules/bluebird/js/main/util.js:26:23)
    at Promise._settlePromiseFromHandler (/home/thomas/repos/kibana/node_modules/bluebird/js/main/promise.js:503:31)
    at Promise._settlePromiseAt (/home/thomas/repos/kibana/node_modules/bluebird/js/main/promise.js:577:18)
    at Promise._settlePromiseAtPostResolution (/home/thomas/repos/kibana/node_modules/bluebird/js/main/promise.js:244:10)
    at Async._drainQueue (/home/thomas/repos/kibana/node_modules/bluebird/js/main/async.js:128:12)
    at Async._drainQueues (/home/thomas/repos/kibana/node_modules/bluebird/js/main/async.js:133:10)
    at Immediate.Async.drainQueues (/home/thomas/repos/kibana/node_modules/bluebird/js/main/async.js:15:14)

The problem is, I can no longer reproduce. These errors are usually gracefully handled (correctly IMO, I don't think we should spam the server with these types of errors).

Perhaps there is an edge-case slipping through the cracks somehow?

Otherwise, LGTM.

@ppisljar
Copy link
Member Author

ppisljar commented Dec 9, 2016

@thomasneirynck i cant reproduce it either. i followed the same pattern rest of timelion uses to throw errors and they usually don't show up in server log but in the client.

@rashidkpc can you reproduce with static line ? something like
.static(0.000000000000000000454352422).yaxis(units=custom:FOO:BAR) works ok for me

@rashidkpc
Copy link
Contributor

@ppisljar You happened to pick one of the rare values that doesn't cause the issue:

weird

I'm not sure if this is even fixable, but shipping with this bug seems really bad since it happens with 0, which is pretty common

screen shot 2016-12-20 at 12 33 13 pm

@ppisljar
Copy link
Member Author

ppisljar commented Jan 13, 2017

i was able to reproduce the bug ... but funny, not with same numbers as you ... for example with 0 it works for me.

what about removing the custom option ? (its the only one causing the issue)

@ppisljar
Copy link
Member Author

its fixed now @rashidkpc please take another look.
i finished doing the same flot is doing to make sure the numbers look fine.

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.

LGTM

@ppisljar ppisljar merged commit 2fce8ea into elastic:master Feb 10, 2017
elastic-jasper added a commit that referenced this pull request Feb 10, 2017
Backports PR #9065

**Commit 1:**
yaxis tick formatters

* Original sha: af24338
* Authored by ppisljar <[email protected]> on 2016-11-14T14:38:12Z

**Commit 2:**
fixing custom format

* Original sha: 21dc660
* Authored by ppisljar <[email protected]> on 2016-11-14T18:21:59Z

**Commit 3:**
adding information to currency help text

* Original sha: 6100b5f
* Authored by ppisljar <[email protected]> on 2016-11-14T19:21:28Z

**Commit 4:**
fixing based on @rashidkpc review

* Original sha: 2b07d6a
* Authored by ppisljar <[email protected]> on 2016-11-15T09:23:10Z

**Commit 5:**
updating based on rashids comments

* Original sha: 8bc012e
* Authored by ppisljar <[email protected]> on 2016-11-17T09:06:51Z

**Commit 6:**
adding some tests

* Original sha: cb8bfd5
* Authored by ppisljar <[email protected]> on 2016-11-17T10:06:02Z

**Commit 7:**
fixing broken yaxis

* Original sha: c1a650c
* Authored by ppisljar <[email protected]> on 2016-11-21T14:40:56Z

**Commit 8:**
fixing broken currency mode

* Original sha: 38d5e12
* Authored by ppisljar <[email protected]> on 2016-11-21T14:41:16Z

**Commit 9:**
adding tick formatters tests

* Original sha: f1f1847
* Authored by ppisljar <[email protected]> on 2016-11-21T14:43:18Z

**Commit 10:**
throw error if currency is not three letter code

* Original sha: deaff2e
* Authored by ppisljar <[email protected]> on 2016-11-23T10:10:45Z

**Commit 11:**
adding server side tests

* Original sha: 1f900ac
* Authored by ppisljar <[email protected]> on 2016-11-23T14:48:33Z

**Commit 12:**
fixing bytes mode

* Original sha: ebe5a6a
* Authored by ppisljar <[email protected]> on 2016-12-09T13:31:54Z

**Commit 13:**
rebasing on master and fixing linting

* Original sha: f1c014d
* Authored by ppisljar <[email protected]> on 2016-12-14T09:44:48Z

**Commit 14:**
fixing custom formatter behaviour.

* Original sha: 1928e6f
* Authored by ppisljar <[email protected]> on 2017-01-13T11:25:27Z
ppisljar pushed a commit that referenced this pull request Feb 11, 2017
Backports PR #9065

**Commit 1:**
yaxis tick formatters

* Original sha: af24338
* Authored by ppisljar <[email protected]> on 2016-11-14T14:38:12Z

**Commit 2:**
fixing custom format

* Original sha: 21dc660
* Authored by ppisljar <[email protected]> on 2016-11-14T18:21:59Z

**Commit 3:**
adding information to currency help text

* Original sha: 6100b5f
* Authored by ppisljar <[email protected]> on 2016-11-14T19:21:28Z

**Commit 4:**
fixing based on @rashidkpc review

* Original sha: 2b07d6a
* Authored by ppisljar <[email protected]> on 2016-11-15T09:23:10Z

**Commit 5:**
updating based on rashids comments

* Original sha: 8bc012e
* Authored by ppisljar <[email protected]> on 2016-11-17T09:06:51Z

**Commit 6:**
adding some tests

* Original sha: cb8bfd5
* Authored by ppisljar <[email protected]> on 2016-11-17T10:06:02Z

**Commit 7:**
fixing broken yaxis

* Original sha: c1a650c
* Authored by ppisljar <[email protected]> on 2016-11-21T14:40:56Z

**Commit 8:**
fixing broken currency mode

* Original sha: 38d5e12
* Authored by ppisljar <[email protected]> on 2016-11-21T14:41:16Z

**Commit 9:**
adding tick formatters tests

* Original sha: f1f1847
* Authored by ppisljar <[email protected]> on 2016-11-21T14:43:18Z

**Commit 10:**
throw error if currency is not three letter code

* Original sha: deaff2e
* Authored by ppisljar <[email protected]> on 2016-11-23T10:10:45Z

**Commit 11:**
adding server side tests

* Original sha: 1f900ac
* Authored by ppisljar <[email protected]> on 2016-11-23T14:48:33Z

**Commit 12:**
fixing bytes mode

* Original sha: ebe5a6a
* Authored by ppisljar <[email protected]> on 2016-12-09T13:31:54Z

**Commit 13:**
rebasing on master and fixing linting

* Original sha: f1c014d
* Authored by ppisljar <[email protected]> on 2016-12-14T09:44:48Z

**Commit 14:**
fixing custom formatter behaviour.

* Original sha: 1928e6f
* Authored by ppisljar <[email protected]> on 2017-01-13T11:25:27Z
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Timelion Timelion app and visualization Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review Team:Beats v5.4.0 v6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants