Metric label

[acs]

Just activated for Metric vis.
First version needing improvements in the params handling.

Related to: #4065

Video showing the feature working at:

https://youtu.be/NxD22gKM3fo

[elasticsearch-release]

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

[acs]

I have signed the CLA two days ago, but after the PR? Do I need to close this PR and create a new one so the CLA is checked correctly?

[nik9000]

I've updated the CLA checker with your information. Something must have failed on the way from echosign. This comment should trigger a cla-recheck.

[acs]

@nik9000 Thank you very much. Next step, time to find a reviewer for this PR!

[nik9000]

@nik9000 Thank you very much. Next step, time to find a reviewer for this PR!

I can't help there! Sorry about that. My browser-foo is far too weak.

[epixa]

@acs Can you add tests for this?

[acs]

@epixa sure, I can work in adding tests. I need to research a bit how Kibana tests are done and define the tests to be passed, Probably just to add a label to a Metric and check it appears in the Metric vis.

@epixa I am not sure the code follows the Kibana architecture and uses its API correctly. For example, making the vis dirty modifying directly $scope.vis.dirty. The core review ... will be done after the testing addition?

[epixa]

@acs Tests make it much easier for reviewers to go through a pull request which helps expedite the review process and get features into Kibana faster and more reliably. That said, in this case if you're concerned that the approach taken here won't be well received, then it's probably worth us taking a look at this in more detail before you write all of the tests for it.

That said, if there are any areas of the code you are comfortable testing, I'd recommend getting a jump start on that, as we ask for tests for all new features at this point.

[epixa]

@acs The approach here is a good proof of concept, but I think this needs to be applied in a more general way so it can be used to label any visualizations instead of just metrics. This would mean you don't actually need to conditionally show/handle the labels for metrics only, and you could lean on the existing labeling logic to handle the custom labels themselves.

If every AggConfig object had an optional customLabel, then you could simply modify the makeLabel function to conditionally check for customLabel. So:

https://github.com/elastic/kibana/blob/master/src/ui/public/Vis/AggConfig.js#L261-L265

would become something like:

AggConfig.prototype.makeLabel = function () {
  if (this.customLabel) return this.customLabel;
  if (!this.type) return '';
  var pre = (_.get(this.vis, 'params.mode') === 'percentage') ? 'Percentage of ' : '';
  return pre += this.type.makeLabel(this);
};

Does that make sense?

[acs]

@epixa great suggestion. I plan to work following your advice next days. Stay tuned!

[acs]

@epixa now is working as you proposed. There are some tidbits yet to be fixed (pie chart does not work). I will try to continue testing all visualizations to be sure all is working as expected. Any comments about then code and how it is implemented will be really appreciated. Next week I plan to continue working in KIbana, so it is a great moment to try to finish the work in this task.

[epixa]

This is better, but I think more of this logic should be moved into AggConfig, and params doesn't need to be leaned on at all. I propose two core changes:

• a new property directly on AggConfig for the custom label rather than relying on params
• no longer maintaining a map of labels in agg.js since $scope.agg is an instance of AggConfig and thus will have access to the current label via makeLabel().

[acs]

@epixa working on it. Not sure if adding a new property to AggConfig will have persistence. But I will play a bit with it and update the PR!

[epixa]

@acs We may need to do some tweaking to make the persistence happen, but I'm sure we can make it work.

[acs]

@epixa I have added customLabel as an AggConfig param and it has persistence for free. But I am right now understanding howto to add a default param to all AggConfig. I am doing it to understand better AggConfig, params and so on.

But you prefer to use directly a property for AggConfig. So time to follow this approach.

[acs]

@epixa I have created a new property in AggConfig:

function AggConfig(vis, opts) {
      var self = this;

      self.id = String(opts.id || AggConfig.nextId(vis.aggs));
      self.vis = vis;
      self.customLabel = 'Custom Label';

and in agg.html I use it with:

  <div class="form-group" ng-show="showAggLabel">
    <label>Label {{agg.id}} {{agg.customLabel}}</label>
    <input
      ng-model="agg.customLabel"

but it seems that "agg" in the scope is filled with AggConfig data (initial value for self.customLabel appears in the input text for the label), but then, there is no sync between agg in the ng-model and AggConfig properties. agg.customLabel value is not set to self.customLabel in AggConfig.

I am working with the params approach which is working nicely which is pretty similar to the property approach, so we can review a more complete version of the PR.

[acs]

@epixa using the same approach than JSON input param, the solution is much better. You get the custom label edit field automatically so no need to add extra stuff in the GUI.

It could be cool to just add this param to Advances, so it does not appear always, but in "Metric" there is no "Advanced" option, like in Bucket.

Not sure in UX guys will accept to have another input text field in all aggs forms, But I think for the rest, this is the right solution. What do you think?

If you want, I can create another PR cleaner from master to avoid the noise of adding and removing code.

[epixa]

jenkins, test it

[acs]

@epixa added tests for all features in AggConfig.prototype.makeLabel.

[epixa]

Awesome! I'll give this a whirl today before passing it either back to you for changes or to another Elastic dev for a final pair of eyes.

[epixa]

jenkins, test it

[epixa]

LGTM

[rashidkpc]

LGTM

[acs]

Great guys! Thank you for your support in this first PR merged to Kibana. Time to continue working. Really happy.

[rashidkpc]

@acs this doesn't apply cleanly on 4.x. If you want to bp this to the 4.x branch we'll get it into 4.5.0, otherwise it'll go into 5.0.0

[epixa]

Thanks for the submission @acs, and thanks for seeing it through until the end.

[rashidkpc]

It appear this has no effect on the filters agg, though is still present there.

[acs]

@rashidkpc I will check the 4.x merging to try to fix it, and also the filters agg to see how to hide it.

[rashidkpc]

@acs Fix for the filter thing is here: #6087

[acs]

@rashidkpc nice. I will focus then in 4.x issue.

[acs]

@rashidkpc bp to the 4.x branch done #6123

[nikmolnar]

This doesn't seem to work for percentile aggregations. There's a "Custom Label" param, but the column headers still show up as "90th percentile of response_time", etc.

[acs]

@nikmolnar working on it! Hmmm, in kibana master it is already fixed.