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.

Sign up for GitHub

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

Jump to bottom

Loki: Add improvements to loki label browser #59387

Merged
merged 5 commits into from Nov 29, 2022
Merged

Loki: Add improvements to loki label browser #59387

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b3de9f4
improvements
gwdawson Nov 28, 2022
c4ceef8
refactor label browser modal
gwdawson Nov 29, 2022
31b5b19
feat(label-browser-modal): fetch labels on modal open
matyax Nov 29, 2022
6e2eb97
apply suggestions
gwdawson Nov 29, 2022
2449f93
check for log labels after languageProvider start
gwdawson Nov 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
51 changes: 12 additions & 39 deletions public/app/plugins/datasource/loki/components/LokiQueryEditor.tsx
View file
Open in desktop
Expand Up @@ -31,7 +31,6 @@ export const LokiQueryEditor = React.memo<LokiQueryEditorProps>((props) => {
const [queryPatternsModalOpen, setQueryPatternsModalOpen] = useState(false);
const [dataIsStale, setDataIsStale] = useState(false);
const [labelBrowserVisible, setLabelBrowserVisible] = useState(false);
const [labelsLoaded, setLabelsLoaded] = useState(false);
const { flag: explain, setFlag: setExplain } = useFlag(lokiQueryEditorExplainKey);

const query = getQueryWithDefaults(props.query);
Expand Down Expand Up @@ -73,30 +72,10 @@ export const LokiQueryEditor = React.memo<LokiQueryEditorProps>((props) => {
onChange(query);
};

const onClickChooserButton = () => {
const onClickLabelBrowserButton = () => {
setLabelBrowserVisible((visible) => !visible);
};

const getChooserText = (logLabelsLoaded: boolean, hasLogLabels: boolean) => {
if (!logLabelsLoaded) {
return 'Loading labels...';
}
if (!hasLogLabels) {
return '(No labels found)';
}
return 'Label browser';
};

useEffect(() => {
datasource.languageProvider.start().then(() => {
setLabelsLoaded(true);
});
}, [datasource]);

const hasLogLabels = datasource.languageProvider.getLabelKeys().length > 0;
const labelBrowserText = getChooserText(labelsLoaded, hasLogLabels);
const buttonDisabled = !(labelsLoaded && hasLogLabels);

return (
<>
<ConfirmModal
Expand All @@ -119,16 +98,16 @@ export const LokiQueryEditor = React.memo<LokiQueryEditorProps>((props) => {
onChange={onChange}
onAddQuery={onAddQuery}
/>
<LabelBrowserModal
isOpen={labelBrowserVisible}
datasource={datasource}
query={query}
app={app}
onClose={() => setLabelBrowserVisible(false)}
onChange={onChangeInternal}
onRunQuery={onRunQuery}
/>
<EditorHeader>
<LabelBrowserModal
isOpen={labelBrowserVisible}
languageProvider={datasource.languageProvider}
query={query}
app={app}
onClose={() => setLabelBrowserVisible(false)}
onChange={onChangeInternal}
onRunQuery={onRunQuery}
/>
<Stack gap={1}>
<Button
aria-label={selectors.components.QueryBuilder.queryPatterns}
Expand All @@ -149,14 +128,8 @@ export const LokiQueryEditor = React.memo<LokiQueryEditorProps>((props) => {
>
Kick start your query
</Button>
<Button
variant="secondary"
size="sm"
onClick={onClickChooserButton}
disabled={buttonDisabled}
data-testid="label-browser-button"
>
{labelBrowserText}
<Button variant="secondary" size="sm" onClick={onClickLabelBrowserButton} data-testid="label-browser-button">
Label browser
</Button>
</Stack>
<QueryHeaderSwitch label="Explain" value={explain} onChange={onExplainChange} />
Expand Down
10 changes: 7 additions & 3 deletions public/app/plugins/datasource/loki/querybuilder/components/LabelBrowserModal.test.tsx
View file
Open in desktop
Expand Up @@ -20,7 +20,7 @@ describe('LabelBrowserModal', () => {

props = {
isOpen: true,
languageProvider: datasource.languageProvider,
datasource: datasource,
query: {} as LokiQuery,
onClose: jest.fn(),
onChange: jest.fn(),
Expand All @@ -30,13 +30,17 @@ describe('LabelBrowserModal', () => {
jest.spyOn(datasource, 'metadataRequest').mockResolvedValue({});
});

it('renders the label browser modal when open', () => {
it('renders the label browser modal when open', async () => {
render(<LabelBrowserModal {...props} />);

expect(await screen.findByText(/Loading/)).not.toBeInTheDocument();

expect(screen.getByRole('heading', { name: /label browser/i })).toBeInTheDocument();
});

it("doesn't render the label browser modal when closed", () => {
it("doesn't render the label browser modal when closed", async () => {
render(<LabelBrowserModal {...props} isOpen={false} />);

expect(screen.queryByRole('heading', { name: /label browser/i })).toBeNull();
});
});
56 changes: 35 additions & 21 deletions public/app/plugins/datasource/loki/querybuilder/components/LabelBrowserModal.tsx
View file
Open in desktop
@@ -1,16 +1,16 @@
import React from 'react';
import React, { useState, useEffect } from 'react';

import { CoreApp } from '@grafana/data';
import { Modal } from '@grafana/ui';
import { LoadingPlaceholder, Modal } from '@grafana/ui';
import { LocalStorageValueProvider } from 'app/core/components/LocalStorageValueProvider';

import LanguageProvider from '../../LanguageProvider';
import { LokiLabelBrowser } from '../../components/LokiLabelBrowser';
import { LokiDatasource } from '../../datasource';
import { LokiQuery } from '../../types';

export interface Props {
isOpen: boolean;
languageProvider: LanguageProvider;
datasource: LokiDatasource;
query: LokiQuery;
app?: CoreApp;
onClose: () => void;
Expand All @@ -19,13 +19,23 @@ export interface Props {
}

export const LabelBrowserModal = (props: Props) => {
const { isOpen, onClose, languageProvider, app } = props;

const { isOpen, onClose, datasource, app } = props;
const [labelsLoaded, setLabelsLoaded] = useState(false);
const LAST_USED_LABELS_KEY = 'grafana.datasources.loki.browser.labels';
const hasLogLabels = datasource.languageProvider.getLabelKeys().length > 0;
gwdawson marked this conversation as resolved.
Show resolved Hide resolved

useEffect(() => {
if (!isOpen) {
return;
}

datasource.languageProvider.start().then(() => {
setLabelsLoaded(true);
});
}, [datasource, isOpen]);

const changeQuery = (value: string) => {
const { query, onChange, onRunQuery } = props;

const nextQuery = { ...query, expr: value };
onChange(nextQuery);
onRunQuery();
Expand All @@ -38,20 +48,24 @@ export const LabelBrowserModal = (props: Props) => {

return (
<Modal isOpen={isOpen} title="Label browser" onDismiss={onClose}>
<LocalStorageValueProvider<string[]> storageKey={LAST_USED_LABELS_KEY} defaultValue={[]}>
{(lastUsedLabels, onLastUsedLabelsSave, onLastUsedLabelsDelete) => {
return (
<LokiLabelBrowser
languageProvider={languageProvider}
onChange={onChange}
lastUsedLabels={lastUsedLabels}
storeLastUsedLabels={onLastUsedLabelsSave}
deleteLastUsedLabels={onLastUsedLabelsDelete}
app={app}
/>
);
}}
</LocalStorageValueProvider>
{!labelsLoaded && <LoadingPlaceholder text="Loading labels..." />}
{!hasLogLabels && <p>No labels found.</p>}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{!hasLogLabels && <p>No labels found.</p>}
{labelsLoaded && !hasLogLabels && <p>No labels found.</p>}

Should this be this?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's why sometimes we prefer ternaries over all these booleans:

Suggested change
{!hasLogLabels && <p>No labels found.</p>}
return (
<Modal isOpen={isOpen} title="Label browser" onDismiss={onClose}>
{!labelsLoaded ? (
<LoadingPlaceholder text="Loading labels..." />
) : (
<>
{!hasLogLabels ? (
<p>No labels found.</p>
) : (
<LocalStorageValueProvider<string[]> storageKey={LAST_USED_LABELS_KEY} defaultValue={[]}>
{(lastUsedLabels, onLastUsedLabelsSave, onLastUsedLabelsDelete) => {
return (
<LokiLabelBrowser
languageProvider={datasource.languageProvider}
onChange={onChange}
lastUsedLabels={lastUsedLabels}
storeLastUsedLabels={onLastUsedLabelsSave}
deleteLastUsedLabels={onLastUsedLabelsDelete}
app={app}
/>
);
}}
</LocalStorageValueProvider>
)}
</>
)}
</Modal>
);

Copy link
Member Author

Choose a reason for hiding this comment

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

@matyax something like this was my initial thought but i'm not a huge fan on nested ternaries.

Copy link
Contributor

Choose a reason for hiding this comment

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

When you start seeing these piling up it's usually a better option:

imagen

Copy link
Contributor

@svennergr svennergr Nov 29, 2022 •
edited

Choose a reason for hiding this comment

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

Imo just using early returns could offer best readability here, even though you may need to repeat <Modal isOpen={isOpen} title="Label browser" onDismiss={onClose}> lines.

e.g.:

if (!labelsLoaded) {
    return (
      <Modal isOpen={isOpen} title="Label browser" onDismiss={onClose}>
        <LoadingPlaceholder text="Loading labels..." />
      </Modal>
    );
  }
  if (!hasLogLabels) {
    return (
      <Modal isOpen={isOpen} title="Label browser" onDismiss={onClose}>
        <p>No labels found.</p>
      </Modal>
    );
  }
  return (
    <Modal isOpen={isOpen} title="Label browser" onDismiss={onClose}>
      <LocalStorageValueProvider<string[]> storageKey={LAST_USED_LABELS_KEY} defaultValue={[]}>
        {(lastUsedLabels, onLastUsedLabelsSave, onLastUsedLabelsDelete) => {
          return (
            <LokiLabelBrowser
              languageProvider={datasource.languageProvider}
              onChange={onChange}
              lastUsedLabels={lastUsedLabels}
              storeLastUsedLabels={onLastUsedLabelsSave}
              deleteLastUsedLabels={onLastUsedLabelsDelete}
              app={app}
            />
          );
        }}
      </LocalStorageValueProvider>
    </Modal>
  );

Copy link
Contributor

Choose a reason for hiding this comment

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

That's also a good option. What I don't like about that is 1) mounting and unmounting a Modal component during a transition (loading -> loaded with or without labels) and 2) having to 3x <Modal isOpen={isOpen} title="Label browser" onDismiss={onClose}>.

Copy link
Member

Choose a reason for hiding this comment

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

Then what about keeping modal, just changing the modal content:

  const LogBrowserContent = () => {
    if (!labelsLoaded) {
      return <LoadingPlaceholder text="Loading labels..." />;
    }

    if (labelsLoaded && !hasLogLabels) {
      return <div>No labels found.</div>;
    }

    return (
      <LocalStorageValueProvider<string[]> storageKey={LAST_USED_LABELS_KEY} defaultValue={[]}>
        {(lastUsedLabels, onLastUsedLabelsSave, onLastUsedLabelsDelete) => {
          return (
            <LokiLabelBrowser
              languageProvider={datasource.languageProvider}
              onChange={onChange}
              lastUsedLabels={lastUsedLabels}
              storeLastUsedLabels={onLastUsedLabelsSave}
              deleteLastUsedLabels={onLastUsedLabelsDelete}
              app={app}
            />
          );
        }}
      </LocalStorageValueProvider>
    );
  };

  return (
    <Modal isOpen={isOpen} title="Label browser" onDismiss={onClose}>
      <LogBrowserContent />
    </Modal>
  );```

{labelsLoaded && hasLogLabels && (
<LocalStorageValueProvider<string[]> storageKey={LAST_USED_LABELS_KEY} defaultValue={[]}>
{(lastUsedLabels, onLastUsedLabelsSave, onLastUsedLabelsDelete) => {
return (
<LokiLabelBrowser
languageProvider={datasource.languageProvider}
onChange={onChange}
lastUsedLabels={lastUsedLabels}
storeLastUsedLabels={onLastUsedLabelsSave}
deleteLastUsedLabels={onLastUsedLabelsDelete}
app={app}
/>
);
}}
</LocalStorageValueProvider>
)}
</Modal>
);
};