Skip to content

Use the customMetric's formatter for pipeline aggregations#11933

Merged
thomasneirynck merged 2 commits intoelastic:masterfrom
trevan:formatting-pipeline-aggs
Jul 19, 2017
Merged

Use the customMetric's formatter for pipeline aggregations#11933
thomasneirynck merged 2 commits intoelastic:masterfrom
trevan:formatting-pipeline-aggs

Conversation

@trevan
Copy link
Copy Markdown
Contributor

@trevan trevan commented May 19, 2017

The pipeline aggregations weren't using the underlying metric's formatter.

@elasticmachine
Copy link
Copy Markdown
Contributor

Can one of the admins verify this patch?

@thomasneirynck
Copy link
Copy Markdown
Contributor

jenkins, test

@thomasneirynck thomasneirynck added :Discovery Feature:Visualizations Generic visualization features (in case no more specific feature label is available) labels May 23, 2017
@thomasneirynck
Copy link
Copy Markdown
Contributor

jenkins, test this

Copy link
Copy Markdown
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.

thanks @trevan, looking good as far as I can tell.

Not an expert on pipeline-aggs though. @ppisljar, can you take a second look?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When do we hit this code-path? Not a 100% familiar with htis, but don't all parent pipelines require a custom-metric?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you can also use a predefined one.

Copy link
Copy Markdown
Contributor

@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.

LGTM, wanna add a simple test as well ?

thanks a lot trevan!

Copy link
Copy Markdown
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.

hey trevan, good for me too (in case that wasn't clear), but would be helpful if you can add corresponding test. thx!

@trevan
Copy link
Copy Markdown
Contributor Author

trevan commented Jul 18, 2017

@ppisljar, @thomasneirynck, sorry it took so long. I finally added tests.

@thomasneirynck
Copy link
Copy Markdown
Contributor

thx!

jenkins, test this

@thomasneirynck
Copy link
Copy Markdown
Contributor

CI failing for most obscure of reasons. https://kibana-ci.elastic.co/job/elastic+kibana+pull-request+multijob-selenium/6095/console

jenkins, test this

@thomasneirynck
Copy link
Copy Markdown
Contributor

jenkins, test this

@thomasneirynck
Copy link
Copy Markdown
Contributor

@trevan could you rebase with master, and fix the merge conflict?
should be pretty easy, adding a comma after getFormat: siblingPipelineHelper.getFormat, thx.

@trevan
Copy link
Copy Markdown
Contributor Author

trevan commented Jul 19, 2017

I rebased.

@thomasneirynck
Copy link
Copy Markdown
Contributor

jenkins, test this

@thomasneirynck
Copy link
Copy Markdown
Contributor

Thx!

thomasneirynck pushed a commit to thomasneirynck/kibana that referenced this pull request Jul 20, 2017
@thomasneirynck
Copy link
Copy Markdown
Contributor

thomasneirynck commented Jul 20, 2017

Backports:
5.x: #13018
5.5: #13029

thomasneirynck added a commit that referenced this pull request Jul 21, 2017
thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Jul 21, 2017
thomasneirynck added a commit that referenced this pull request Jul 21, 2017
@trevan trevan deleted the formatting-pipeline-aggs branch December 19, 2017 17:56
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 v5.5.2 v5.6.0 v6.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants