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

Use Explore's Prometheus editor in dashboard panel edit #15364

Merged
merged 11 commits into from Jun 24, 2019

Conversation

davkal
Copy link
Contributor

@davkal davkal commented Feb 11, 2019

  • adds PromQueryField to query editor partial
  • query is being pushed down for the initial value, and changes coming from the editor bubble up and override the panel target

@@ -1,6 +1,8 @@
import { ComponentClass } from 'react';
import { PanelProps, PanelOptionsProps } from './panel';
import { DataQueryOptions, DataQuery, DataQueryResponse, QueryHint, QueryFixAction } from './datasource';
import { TimeRange } from './time';
import { PanelModel } from 'app/features/dashboard/state';
Copy link
Member

@torkelo torkelo Feb 14, 2019

Choose a reason for hiding this comment

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

You cannot import stuff from the app from grafana ui

@@ -49,7 +51,9 @@ export interface ExploreDataSourceApi<TQuery extends DataQuery = DataQuery> exte

export interface QueryEditorProps<DSType extends DataSourceApi, TQuery extends DataQuery> {
datasource: DSType;
panel?: PanelModel;
Copy link
Member

@torkelo torkelo Feb 14, 2019

Choose a reason for hiding this comment

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

Don’t want plugins to have access to the panel model , why do query editors need this ? They could need panel plugin meta info like supported data formats , but want to shield plugins from the actual panel model as this has some legacy things like event emitter and persisted state that we will likely want to change in the future without affecting plugins

@davkal davkal force-pushed the davkal/prom-edit-unify branch 2 times, most recently from 2edbfc3 to e4a7434 Compare Feb 19, 2019
@CLAassistant
Copy link

@CLAassistant CLAassistant commented Feb 19, 2019

CLA assistant check
All committers have signed the CLA.

@torkelo
Copy link
Member

@torkelo torkelo commented Mar 13, 2019

Is this still WIP? what is the state? let me know if this can be tested and & merged. Or is it waiting for getting the latest query response added to the QueryEditorProps interface?

@davkal
Copy link
Contributor Author

@davkal davkal commented Mar 15, 2019

Basic functionality is done. Needs tests. Hope to finish off next week.

@davkal davkal force-pushed the davkal/prom-edit-unify branch from 08234eb to 43b48e0 Compare Mar 20, 2019
@davkal davkal changed the title [WIP] Use Explore's Prometheus editor in dashboard panel edit Use Explore's Prometheus editor in dashboard panel edit Mar 20, 2019
@davkal
Copy link
Contributor Author

@davkal davkal commented Mar 20, 2019

This is ready for an initial review.

Given that the editor component no longer has access to the panel controller, we're missing two things for the external link generation: 1) dynamic step interval from the panel, 2) template variables updates

The dynamic step interval is probably less important than the template variables. It's going to be very frustrating when someone exits to Prometheus and starts debugging, only to find out that they are operating on a different cluster/datacenter/instance. Any idea on how we can force a render of the link when the variables update? @torkelo

onLegendChange = (e: React.SyntheticEvent<HTMLInputElement>) => {
const legendFormat = e.currentTarget.value;
this.query.legendFormat = legendFormat;
this.setState({ legendFormat });
Copy link
Member

@torkelo torkelo Mar 20, 2019

Choose a reason for hiding this comment

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

should this not also trigger a run query so you see the effect?


export type Props = QueryEditorProps<PrometheusDatasource, PromQuery>;

const FORMAT_OPTIONS: SelectOptionItem[] = [
Copy link
Member

@torkelo torkelo Mar 20, 2019

Choose a reason for hiding this comment

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

why caps?

@torkelo
Copy link
Member

@torkelo torkelo commented Mar 27, 2019

Did a quick poc for adding queryResponse & queryError to react query editor props:
#16233

@davkal
Copy link
Contributor Author

@davkal davkal commented Apr 1, 2019

@torkelo any guidance on how panel editors should react to a user changing template variables? This is needed for the external link creation.
Since the editor has no access to the dashboard, they can't subscribe to 'template-variable-value-updated' their events.

@torkelo
Copy link
Member

@torkelo torkelo commented Apr 1, 2019

We need pass variables in as props, but with the PR I linked to above you will also get a new query response as queries are executed in variable change, but to show variables in auto complete suggestions we need to pass variables in as props as well

@davkal
Copy link
Contributor Author

@davkal davkal commented Apr 1, 2019

@torkelo Great, I can trigger a render then. Will wait for #16233

@torkelo
Copy link
Member

@torkelo torkelo commented Apr 2, 2019

That PR (queryRespone & queryError) is now available in master, merged via this other PR from ryan #16306

But added refId filtering to both queryError and queryResponse, so we need to add refId to errors and series responses in the Prometheus response handling.

@torkelo
Copy link
Member

@torkelo torkelo commented Apr 2, 2019

Now that we have queryResponse & queryError in the QueryEditorProps would be interesting as next steps to review how we can align & remove the Prometheus/loki specific from the Explore query interfaces:

export interface ExploreQueryFieldProps<DSType extends DataSourceApi, TQuery extends DataQuery> {
  datasource: DSType;
  query: TQuery;
  error?: string | JSX.Element;
  hint?: QueryHint;
  history: any[];
  onExecuteQuery?: () => void;
  onQueryChange?: (value: TQuery) => void;
  onExecuteHint?: (action: QueryFixAction) => void;
}

Would like to remove hint & onExecuteHint as they seem internal details that can be handled inside the QueryEditor. For example datasource.getQueryHints could be an internal Prometheus specific function that the QueryEditor calls, does not need to be a public function on DataSourceApi.

@davkal
Copy link
Contributor Author

@davkal davkal commented Jun 20, 2019

LGTM. Thanks for taking this on @hugohaggmark

IMO this can go in as is. Someone other than me needs to give a review (I opened the PR.)

The one piece of functionality we're missing over the existing editor is start/end awareness of metadata queries (when requesting labels, start and end from the visible range should be sent as well). We should add this in a follow-up PR.

Copy link
Contributor

@hugohaggmark hugohaggmark left a comment

LGTM!

@hugohaggmark hugohaggmark merged commit 4ddeb94 into master Jun 24, 2019
2 checks passed
Observability (deprecated, use Observability Squad) automation moved this from Under review to Done Jun 24, 2019
@hugohaggmark hugohaggmark deleted the davkal/prom-edit-unify branch Jun 24, 2019
markelog added a commit that referenced this issue Jun 24, 2019
* master:
  TimePicker: New time picker dropdown & custom range UI (#16811)
  RemoteCache: redis connection string parsing test (#17702)
  Fix link in pkg/README (#17714)
  Dashboard: Use Explore's Prometheus editor in dashboard panel edit (#15364)
  Settings: Fix typo in defaults.ini (#17707)
  Project: Adds a security policy (#17698)
  Project: Adds support resource docs (#17699)
  Document issue triage process (#17669)
  noImplicitAny: slate (#17681)
  config: fix connstr for remote_cache (#17675)
ryantxu added a commit to ryantxu/grafana that referenced this issue Jun 24, 2019
…-mapping-to-field

* grafana/master:
  Elasticsearch: Visualize logs in Explore (grafana#17605)
  Grafana-CLI: Wrapper for `grafana-cli` within RPM/DEB packages and config/homepath are now global flags (grafana#17695)
  Add guidelines for SQL date comparisons (grafana#17732)
  Docs: clarified usage of go get and go mod (grafana#17637)
  Project: Issue triage doc improvement (grafana#17709)
  Improvement: Grafana release process minor improvements (grafana#17661)
  TimePicker: New time picker dropdown & custom range UI (grafana#16811)
  RemoteCache: redis connection string parsing test (grafana#17702)
  Fix link in pkg/README (grafana#17714)
  Dashboard: Use Explore's Prometheus editor in dashboard panel edit (grafana#15364)
  Settings: Fix typo in defaults.ini (grafana#17707)
  Project: Adds a security policy (grafana#17698)
  Project: Adds support resource docs (grafana#17699)
  Document issue triage process (grafana#17669)
  noImplicitAny: slate (grafana#17681)
  config: fix connstr for remote_cache (grafana#17675)
  Explore: Improves performance of Logs element by limiting re-rendering (grafana#17685)
  Docs: Flag serve_from_sub_path as available in 6.3 (grafana#17674)
  @grafana/runtime: expose config and loadPluginCss (grafana#17655)
  noImplicitAny: Fix basic errors (grafana#17668)
ryantxu added a commit to ryantxu/grafana that referenced this issue Jun 25, 2019
* grafana/master:
  Elasticsearch: Visualize logs in Explore (grafana#17605)
  Grafana-CLI: Wrapper for `grafana-cli` within RPM/DEB packages and config/homepath are now global flags (grafana#17695)
  Add guidelines for SQL date comparisons (grafana#17732)
  Docs: clarified usage of go get and go mod (grafana#17637)
  Project: Issue triage doc improvement (grafana#17709)
  Improvement: Grafana release process minor improvements (grafana#17661)
  TimePicker: New time picker dropdown & custom range UI (grafana#16811)
  RemoteCache: redis connection string parsing test (grafana#17702)
  Fix link in pkg/README (grafana#17714)
  Dashboard: Use Explore's Prometheus editor in dashboard panel edit (grafana#15364)
  Settings: Fix typo in defaults.ini (grafana#17707)
  Project: Adds a security policy (grafana#17698)
  Project: Adds support resource docs (grafana#17699)
  Document issue triage process (grafana#17669)
  noImplicitAny: slate (grafana#17681)
  config: fix connstr for remote_cache (grafana#17675)
  Explore: Improves performance of Logs element by limiting re-rendering (grafana#17685)
  Docs: Flag serve_from_sub_path as available in 6.3 (grafana#17674)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants