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

Elasticsearch: Add custom query variable editor #16697

Closed
wants to merge 6 commits into from

Conversation

marefr
Copy link
Member

@marefr marefr commented Apr 21, 2019

What this PR does / why we need it:
I never remembers the syntax for Elasticsearch template variable queries so decided to have a look at adding a custom query variable editor for Elasticsearch.

Fields:
image

Terms:
image

Which issue(s) this PR fixes:
None at the moment, but maybe should create one?

Special notes for your reviewer:
What's or thoughts regarding snapshot testing? Was unsure so added it for all new components.

Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

Looks good! the use of the blue "query-keyword" form labels feels a bit off as none of the other form labels use it

Think we (soon) need to add TemplateVariables[] input prop to the QueryEditor interface and have more shared code / utils to create selects that have template variables so that not every editor needs to fetch them from templateSrv and construct the option list.

There is https://github.com/grafana/grafana/blob/master/public/app/core/components/Select/MetricSelect.tsx

that is a start that Erik did that Stackdriver is using, maybe that can be used here?

@marefr
Copy link
Member Author

marefr commented Apr 29, 2019

GitHub misses to send notifications sometimes 😢 Had missed your review completely.

the use of the blue "query-keyword" form labels feels a bit off as none of the other form labels use it

Stackdriver query editor uses that that's why, but I'm glad to remove that?

Think we (soon) need to add TemplateVariables[] input prop to the QueryEditor interface and have more shared code / utils to create selects that have template variables so that not every editor needs to fetch them from templateSrv and construct the option list.

Yeah that's probably a good idea.

There is https://github.com/grafana/grafana/blob/master/public/app/core/components/Select/MetricSelect.tsx

that is a start that Erik did that Stackdriver is using, maybe that can be used here?

I looked at the MetricSelect but didn't think that was searchable and supported. Gonna have another look at that.

@marefr marefr requested a review from torkelo May 8, 2019 19:30
@torkelo
Copy link
Member

torkelo commented May 9, 2019

So question is if we should switch to object model now for the variable query instead of a string like stackdriver is using. Would require migration or handling both string and query object in the data source.

The VariableQueryProps interface looks strange, onChange(query: any, definition: string)

and template editor stores a definition prop on the query variable persistence object that is not used anywhere. Think this is s remnant from a previous design idea from Erik where there was a query or a definition object but looks not be used anywhere.

@marefr
Copy link
Member Author

marefr commented May 9, 2019

So question is if we should switch to object model now for the variable query instead of a string like stackdriver is using. Would require migration or handling both string and query object in the data source.

Not sure I follow. Are you talking about that variable queries is stored in dashboard model as a json string? Tried to store it as object for Elasticsearch but something treats it as string somewhere do didn't work. That's why I've still store it as json string.

The VariableQueryProps interface looks strange, onChange(query: any, definition: string)

and template editor stores a definition prop on the query variable persistence object that is not used anywhere. Think this is s remnant from a previous design idea from Erik where there was a query or a definition object but looks not be used anywhere.

It's used to render the definition here:
image

@torkelo
Copy link
Member

torkelo commented May 9, 2019

The definition string still feels a bit strange like persisting a view model in the dashboard json.

What expects the Elasticsearch template variable query to be a string? Beside the elasticsearch metricFindQuery ?

Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

But I think we can merge this and improve it later. Would be nice to have the query be an object model and not a string now we have this but we could do that later as well.

@marefr
Copy link
Member Author

marefr commented May 21, 2019

Planning to look into what exactly the issue is with using an object model before merging this.

@marcusolsson marcusolsson added area/frontend datasource/Elasticsearch needs investigation for unconfirmed bugs. use type/bug for confirmed bugs, even if they "need" more investigating labels Nov 4, 2019
@stale
Copy link

stale bot commented Dec 4, 2019

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale Issue with no recent activity label Dec 4, 2019
@stale
Copy link

stale bot commented Jan 3, 2020

This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot closed this Jan 3, 2020
@marefr marefr reopened this Jan 10, 2020
@stale stale bot removed the stale Issue with no recent activity label Jan 10, 2020
@stale
Copy link

stale bot commented Jan 24, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale Issue with no recent activity label Jan 24, 2020
@marefr
Copy link
Member Author

marefr commented Jan 27, 2020

Planning to pick this up

@stale stale bot removed the stale Issue with no recent activity label Jan 27, 2020
@stale
Copy link

stale bot commented Feb 10, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale Issue with no recent activity label Feb 10, 2020
@marefr
Copy link
Member Author

marefr commented Feb 10, 2020

Planning to pick this up

@stale stale bot removed the stale Issue with no recent activity label Feb 10, 2020
@stale
Copy link

stale bot commented Feb 24, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale Issue with no recent activity label Feb 24, 2020
@stale
Copy link

stale bot commented Mar 25, 2020

This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot closed this Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend datasource/Elasticsearch needs investigation for unconfirmed bugs. use type/bug for confirmed bugs, even if they "need" more investigating stale Issue with no recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants