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

top_hit metric aggregation: make string typed fields and 'Concatenate' option compatibles with all visualizations which type is 'table' #23462

Closed
wants to merge 1 commit into from

Conversation

fbaligand
Copy link
Contributor

@fbaligand fbaligand commented Sep 24, 2018

Currently, in 'top_hit' aggregation, string typed fields and 'Concatenate' option are compatibles only with the visualization named 'table'.

This PR lets these two features be available to every visualization which type is 'table'.

This is particularly useful for these plugins :
http://github.com/fbaligand/kibana-enhanced-table
https://github.com/seadiaz/computed-columns
https://github.com/dlumbrer/kbn_searchtables

@elasticmachine
Copy link
Contributor

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@Bargs Bargs added review Feature:Visualizations Generic visualization features (in case no more specific feature label is available) triage_needed Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Sep 25, 2018
@timroes
Copy link
Contributor

timroes commented Sep 26, 2018

Jenkins, test this

@timroes timroes self-assigned this Sep 26, 2018
@timroes
Copy link
Contributor

timroes commented Sep 26, 2018

@fbaligand thanks for that PR. We really need something like that, but unfortunately that implementation won't be future proof enough. We are switching towards using the Canvas pipeline in all of Kibana (#19813), the querying infrastructure (AggConfig, etc.) will be decoupled from vis (which will vanish completely) or anything visualization specific. So we later won't have the ability to access vis from any aggregation anymore - meaning that also the whole check on the vis.type needs to be gone.

Everything that would switch some behavior like that needs to be a parameter to the AggConfig somehow. Unfortunately we don't have an option yet, so make the vis type specify options on the AggConfig. Instead the idea is adding something to the Schema the visualization registers, that allows configuration on the AggConfig.

Unfortunately we haven't a clear idea about that yet. Since I see why you need this, I try to find some time beginning of next week to sync about that topic, and would ideally also include you to that conversation.

@timroes
Copy link
Contributor

timroes commented Sep 26, 2018

cc @ppisljar

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@fbaligand
Copy link
Contributor Author

Hi @timroes,

Thanks for your detailed explanation and insights on what's coming on.
The idea of using "Schema" looks like a good idea.

Then, I guess that whole Kibana refactoring to Canvas is a huge work that should take a lot of time, probably available not before next major version. This PR would let to have a quick win, maybe available in Kibana 6.5, waiting the full refactoring. And it does not change the code a lot.
What do you think about ?

@fbaligand
Copy link
Contributor Author

Hi @timroes

You said you will try to find this week a way to nicely fulfill this PR feature, that is compatible with new Kibana pipeline.
Did you find some time to work on it ?

@joshdover
Copy link
Contributor

@fbaligand Apologize for our delay, we're currently at our bi-annual all hands conference doing planning. Tim should be able to get to this next week!

@fbaligand
Copy link
Contributor Author

OK, thanks for the info @joshdover !

@elasticmachine
Copy link
Contributor

💔 Build Failed

@fbaligand
Copy link
Contributor Author

Hi @timroes

I ping you on this PR, given that there is still no solution since months.

I truly think this PR is a good first solution to fix the issue for table plugins :

  • it does not add a new feature in Kibana core itself, it is just a different implementation, more clean.
  • if later, the solution to do that is different, this would be just yet another breaking change for plugins (among others).

That's why I think this PR is a nice first implementation.
What do you think about?

@timroes
Copy link
Contributor

timroes commented Jun 28, 2019

Closing this in favor of: #39880

@timroes timroes closed this Jun 28, 2019
@fbaligand fbaligand deleted the top-hit-in-table branch November 28, 2020 15:11
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) review Team:Visualizations Visualization editors, elastic-charts and infrastructure triage_needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants