Skip to content

Commit

Permalink
fix: remove onChange behavior, only run queries on button submit
Browse files Browse the repository at this point in the history
  • Loading branch information
gtk-grafana committed Jan 8, 2025
1 parent db09f1a commit 0c1b45d
Show file tree
Hide file tree
Showing 9 changed files with 69 additions and 117 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ export function LineFilterEditor({
fill={'outline'}
disabled={!lineFilter}
>
Add to filter
Add filter
</Button>
</>
)}
Expand Down
64 changes: 12 additions & 52 deletions src/Components/ServiceScene/LineFilter/LineFilterScene.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -51,24 +51,9 @@ describe('LineFilter', () => {
await act(() => userEvent.type(screen.getByPlaceholderText('Search in log lines'), 'some text'));

Check warning on line 51 in src/Components/ServiceScene/LineFilter/LineFilterScene.test.tsx

View workflow job for this annotation

GitHub Actions / build

'act' is deprecated. https://react.dev/warnings/react-dom-test-utils

expect(await screen.findByDisplayValue('some text')).toBeInTheDocument();
expect(lineFilterVariable.state.filters).toEqual([
{
keyLabel: '0',
key: LineFilterCaseSensitive.caseInsensitive,
operator: LineFilterOp.match,
value: 'some text',
},
]);
expect(lineFilterVariable.getValue()).toBe('|~ `(?i)some text`');
});

test('Escapes the regular expression in the variable', async () => {
render(<scene.Component model={scene} />);

await act(() => userEvent.type(screen.getByPlaceholderText('Search in log lines'), '(characters'));

expect(await screen.findByDisplayValue('(characters')).toBeInTheDocument();
expect(lineFilterVariable.getValue()).toBe('|~ `(?i)\\(characters`');
expect(scene.state.lineFilter).toEqual('some text');
expect(scene.state.regex).toEqual(false);
expect(scene.state.caseSensitive).toEqual(false);
});

test('Unescapes the regular expression from the variable value', async () => {
Expand Down Expand Up @@ -111,16 +96,9 @@ describe('LineFilter', () => {
await act(() => userEvent.type(screen.getByPlaceholderText('Search in log lines'), 'some text'));

Check warning on line 96 in src/Components/ServiceScene/LineFilter/LineFilterScene.test.tsx

View workflow job for this annotation

GitHub Actions / build

'act' is deprecated. https://react.dev/warnings/react-dom-test-utils

expect(await screen.findByDisplayValue('some text')).toBeInTheDocument();
expect(lineFilterVariable.getValue()).toBe('|= `some text`');
});

test('Does not escape user input', async () => {
render(<scene.Component model={scene} />);

await act(() => userEvent.type(screen.getByPlaceholderText('Search in log lines'), '.(characters'));

expect(await screen.findByDisplayValue('.(characters')).toBeInTheDocument();
expect(lineFilterVariable.getValue()).toBe('|= `.(characters`');
expect(scene.state.lineFilter).toEqual('some text');
expect(scene.state.regex).toEqual(false);
expect(scene.state.caseSensitive).toEqual(true);
});

test('Unescapes the regular expression from the variable value', async () => {
Expand Down Expand Up @@ -166,18 +144,9 @@ describe('LineFilter', () => {
await act(() => fireEvent.change(input, { target: { value: string } }));

Check warning on line 144 in src/Components/ServiceScene/LineFilter/LineFilterScene.test.tsx

View workflow job for this annotation

GitHub Actions / build

'act' is deprecated. https://react.dev/warnings/react-dom-test-utils

expect(await screen.findByDisplayValue(string)).toBeInTheDocument();
expect(lineFilterVariable.getValue()).toBe(`|~ \`(?i)${string}\``);
});

test('Does not escape the regular expression', async () => {
render(<scene.Component model={scene} />);

const string = `((25[0-5]|(2[0-4]|1\\d|[1-9]|)\\d)\\.?\\b){4}`;
const input = screen.getByPlaceholderText('Search in log lines');
await act(() => fireEvent.change(input, { target: { value: string } }));

expect(await screen.findByDisplayValue(string)).toBeInTheDocument();
expect(lineFilterVariable.getValue()).toBe(`|~ \`(?i)${string}\``);
expect(scene.state.lineFilter).toEqual(string);
expect(scene.state.regex).toEqual(true);
expect(scene.state.caseSensitive).toEqual(false);
});

test('Unescapes the regular expression from the variable value', async () => {
Expand Down Expand Up @@ -223,18 +192,9 @@ describe('LineFilter', () => {
await act(() => fireEvent.change(input, { target: { value: string } }));

Check warning on line 192 in src/Components/ServiceScene/LineFilter/LineFilterScene.test.tsx

View workflow job for this annotation

GitHub Actions / build

'act' is deprecated. https://react.dev/warnings/react-dom-test-utils

expect(await screen.findByDisplayValue(string)).toBeInTheDocument();
expect(lineFilterVariable.getValue()).toBe(`|~ \`${string}\``);
});

test('Does not escape the regular expression', async () => {
render(<scene.Component model={scene} />);

const string = `((25[0-5]|(2[0-4]|1\\d|[1-9]|)\\d)\\.?\\b){4}`;
const input = screen.getByPlaceholderText('Search in log lines');
await act(() => fireEvent.change(input, { target: { value: string } }));

expect(await screen.findByDisplayValue(string)).toBeInTheDocument();
expect(lineFilterVariable.getValue()).toBe(`|~ \`${string}\``);
expect(scene.state.lineFilter).toEqual(string);
expect(scene.state.regex).toEqual(true);
expect(scene.state.caseSensitive).toEqual(true);
});

test('Unescapes the regular expression from the variable value', async () => {
Expand Down
17 changes: 12 additions & 5 deletions src/Components/ServiceScene/LineFilter/LineFilterScene.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -130,18 +130,24 @@ export class LineFilterScene extends SceneObjectBase<LineFilterState> {

/**
* Updates line filter state
* Note: Updating/debouncing the queries onChange was removed to prevent people from accidentally hammering loki while writing line filters (particularly regex)
* The code has been left in for now as we discussed adding an "edit" mode with a dedicated logs panel with a smaller line limit to let users debug the results as they type
*/
updateFilter(lineFilter: string, debounced = true) {
this.setState({
lineFilter,
});
this.updateInputState(lineFilter);
if (debounced) {
this.updateVariableDebounced(lineFilter);
} else {
this.updateVariable(lineFilter);
}
}

updateInputState(lineFilter: string) {
this.setState({
lineFilter,
});
}

/**
* Update exclusive state, triggers re-query without debounce
*/
Expand All @@ -159,22 +165,23 @@ export class LineFilterScene extends SceneObjectBase<LineFilterState> {
* Clears the state of the local ad-hoc variable.
*/
onSubmitLineFilter = () => {
this.updateFilter(this.state.lineFilter, false);
// Flush any debounced updates before grabbing the filter. Important that this happens before getFilter is called!
this.updateVariableDebounced.flush();

const lineFiltersVariable = getLineFiltersVariable(this);
const existingFilters = lineFiltersVariable.state.filters;
const thisFilter = this.getFilter();

lineFiltersVariable.updateFilters([...existingFilters, thisFilter], { skipPublish: true });
lineFiltersVariable.updateFilters([...existingFilters, thisFilter]);
this.clearVariable();
};

/**
* Passes the input value to the updateFilter method
*/
handleChange = (e: ChangeEvent<HTMLInputElement>) => {
this.updateFilter(e.target.value);
this.updateInputState(e.target.value);
};

/**
Expand Down
30 changes: 22 additions & 8 deletions src/Components/ServiceScene/LogsListScene.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
SceneComponentProps,
SceneFlexItem,
SceneFlexLayout,
sceneGraph,
SceneObjectBase,
SceneObjectState,
SceneObjectUrlSyncConfig,
Expand Down Expand Up @@ -37,13 +38,13 @@ export interface LogsListSceneState extends SceneObjectState {
selectedLine?: SelectedTableRow;
$timeRange?: SceneTimeRangeLike;
displayedFields: string[];
lineFilter?: string;
}

export class LogsListScene extends SceneObjectBase<LogsListSceneState> {
protected _urlSync = new SceneObjectUrlSyncConfig(this, {
keys: ['urlColumns', 'selectedLine', 'visualizationType', 'displayedFields', 'tableLogLineState'],
});
private lineFilterScene?: LineFilterScene = undefined;
private logsPanelScene?: LogsPanelScene = undefined;
constructor(state: Partial<LogsListSceneState>) {
super({
Expand Down Expand Up @@ -145,10 +146,6 @@ export class LogsListScene extends SceneObjectBase<LogsListSceneState> {
);
}

public getLineFilterScene() {
return this.lineFilterScene;
}

private setStateFromUrl(searchParams: URLSearchParams) {
const selectedLineUrl = searchParams.get('selectedLine');
const urlColumnsUrl = searchParams.get('urlColumns');
Expand All @@ -175,6 +172,24 @@ export class LogsListScene extends SceneObjectBase<LogsListSceneState> {
this.setState({
panel: this.getVizPanel(),
});

// Subscribe to line filter state so we can pass the current filter between different viz
if (this.state.panel) {
const lineFilterScenes = sceneGraph.findDescendents(this.state.panel, LineFilterScene);
if (lineFilterScenes.length) {
const lineFilterScene = lineFilterScenes[0];
this._subs.add(
lineFilterScene.subscribeToState((newState, prevState) => {
if (newState.lineFilter !== prevState.lineFilter) {
console.log('setting the damn state', newState.lineFilter);
this.setState({
lineFilter: newState.lineFilter,
});
}
})
);
}
}
};

public setVisualizationType = (type: LogsVisualizationType) => {
Expand All @@ -193,7 +208,6 @@ export class LogsListScene extends SceneObjectBase<LogsListSceneState> {
};

private getVizPanel() {
this.lineFilterScene = new LineFilterScene();
this.logsPanelScene = new LogsPanelScene({});

return new SceneFlexLayout({
Expand All @@ -204,7 +218,7 @@ export class LogsListScene extends SceneObjectBase<LogsListSceneState> {
new SceneFlexLayout({
children: [
new SceneFlexItem({
body: this.lineFilterScene,
body: new LineFilterScene({ lineFilter: this.state.lineFilter }),
xSizing: 'fill',
}),
],
Expand All @@ -216,7 +230,7 @@ export class LogsListScene extends SceneObjectBase<LogsListSceneState> {
]
: [
new SceneFlexItem({
body: this.lineFilterScene,
body: new LineFilterScene({ lineFilter: this.state.lineFilter }),
xSizing: 'fill',
}),
new SceneFlexItem({
Expand Down
4 changes: 2 additions & 2 deletions src/Components/ServiceScene/LogsPanelScene.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -78,15 +78,15 @@ export class LogsPanelScene extends SceneObjectBase<LogsPanelSceneState> {
updateFromUrl(values: SceneObjectUrlValues) {
const stateUpdate: Partial<LogsPanelSceneState> = {};
try {
if (typeof values.sortOrder === 'string') {
if (typeof values.sortOrder === 'string' && values.sortOrder) {
const decodedSortOrder = narrowLogsSortOrder(JSON.parse(values.sortOrder));
if (decodedSortOrder) {
stateUpdate.sortOrder = decodedSortOrder;
this.setLogsVizOption({ sortOrder: decodedSortOrder });
}
}

if (typeof values.wrapLogMessage === 'string') {
if (typeof values.wrapLogMessage === 'string' && values.wrapLogMessage) {
const decodedWrapLogMessage = JSON.parse(values.wrapLogMessage);
if (typeof decodedWrapLogMessage === 'boolean') {
stateUpdate.wrapLogMessage = decodedWrapLogMessage;
Expand Down
24 changes: 0 additions & 24 deletions src/Components/ServiceScene/ServiceScene.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ import {
getLabelsVariable,
getLevelsVariable,
getLineFiltersVariable,
getLineFilterVariable,
getMetadataVariable,
getPatternsVariable,
} from '../../services/variableGetters';
Expand Down Expand Up @@ -305,30 +304,13 @@ export class ServiceScene extends SceneObjectBase<ServiceSceneState> {
this._subs.add(this.subscribeToPatternsVariable());
this._subs.add(this.subscribeToLineFiltersVariable());

if (getDrilldownSlug() !== PageSlugs.logs) {
this.resetPendingLineFilter();
} else {
this._subs.add(this.subscribeToLineFilterVariable());
}

// Update query runner on manual time range change
this._subs.add(this.subscribeToTimeRange());

// Migrations
migrateLineFilterV1(this);
}

/**
* If the user navigates away from the logs scene, but has a pending filter that hasn't been submitted, clear it out
*/
private resetPendingLineFilter() {
// Clear line filters variable
const variable = getLineFilterVariable(this);
variable.setState({
filters: [],
});
}

private subscribeToPatternsVariable() {
return getPatternsVariable(this).subscribeToState((newState, prevState) => {
if (newState.value !== prevState.value) {
Expand All @@ -338,12 +320,6 @@ export class ServiceScene extends SceneObjectBase<ServiceSceneState> {
});
}

private subscribeToLineFilterVariable() {
return getLineFilterVariable(this).subscribeToEvent(SceneVariableValueChangedEvent, () => {
this.state.$logsCount?.runQueries();
});
}

private subscribeToLineFiltersVariable() {
return getLineFiltersVariable(this).subscribeToEvent(SceneVariableValueChangedEvent, () => {
this.state.$logsCount?.runQueries();
Expand Down
9 changes: 4 additions & 5 deletions src/services/expressions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {
VAR_FIELDS_EXPR,
VAR_LABELS_EXPR,
VAR_LINE_FILTERS_EXPR,
VAR_LINE_FILTER_EXPR,
VAR_METADATA_EXPR,
VAR_PATTERNS_EXPR,
} from './variables';
Expand Down Expand Up @@ -38,14 +37,14 @@ export function getTimeSeriesExpr(sceneRef: SceneObject, streamSelectorName: str
// if we have fields, we also need to add parsers
if (fieldFilters.length) {
if (parser === 'mixed') {
return `sum(count_over_time({${VAR_LABELS_EXPR}} ${metadataExpressionToAdd} ${VAR_METADATA_EXPR} ${VAR_PATTERNS_EXPR} ${VAR_LINE_FILTER_EXPR} ${VAR_LINE_FILTERS_EXPR} ${MIXED_FORMAT_EXPR} ${VAR_FIELDS_EXPR} [$__auto])) by (${streamSelectorName})`;
return `sum(count_over_time({${VAR_LABELS_EXPR}} ${metadataExpressionToAdd} ${VAR_METADATA_EXPR} ${VAR_PATTERNS_EXPR} ${VAR_LINE_FILTERS_EXPR} ${MIXED_FORMAT_EXPR} ${VAR_FIELDS_EXPR} [$__auto])) by (${streamSelectorName})`;
}
if (parser === 'json') {
return `sum(count_over_time({${VAR_LABELS_EXPR}} ${metadataExpressionToAdd} ${VAR_METADATA_EXPR} ${VAR_PATTERNS_EXPR} ${VAR_LINE_FILTER_EXPR} ${VAR_LINE_FILTERS_EXPR} ${JSON_FORMAT_EXPR} ${VAR_FIELDS_EXPR} [$__auto])) by (${streamSelectorName})`;
return `sum(count_over_time({${VAR_LABELS_EXPR}} ${metadataExpressionToAdd} ${VAR_METADATA_EXPR} ${VAR_PATTERNS_EXPR} ${VAR_LINE_FILTERS_EXPR} ${JSON_FORMAT_EXPR} ${VAR_FIELDS_EXPR} [$__auto])) by (${streamSelectorName})`;
}
if (parser === 'logfmt') {
return `sum(count_over_time({${VAR_LABELS_EXPR}} ${metadataExpressionToAdd} ${VAR_METADATA_EXPR} ${VAR_PATTERNS_EXPR} ${VAR_LINE_FILTER_EXPR} ${VAR_LINE_FILTERS_EXPR} ${LOGS_FORMAT_EXPR} ${VAR_FIELDS_EXPR} [$__auto])) by (${streamSelectorName})`;
return `sum(count_over_time({${VAR_LABELS_EXPR}} ${metadataExpressionToAdd} ${VAR_METADATA_EXPR} ${VAR_PATTERNS_EXPR} ${VAR_LINE_FILTERS_EXPR} ${LOGS_FORMAT_EXPR} ${VAR_FIELDS_EXPR} [$__auto])) by (${streamSelectorName})`;
}
}
return `sum(count_over_time({${VAR_LABELS_EXPR}} ${metadataExpressionToAdd} ${VAR_METADATA_EXPR} ${VAR_PATTERNS_EXPR} ${VAR_LINE_FILTER_EXPR} ${VAR_LINE_FILTERS_EXPR} ${VAR_FIELDS_EXPR} [$__auto])) by (${streamSelectorName})`;
return `sum(count_over_time({${VAR_LABELS_EXPR}} ${metadataExpressionToAdd} ${VAR_METADATA_EXPR} ${VAR_PATTERNS_EXPR} ${VAR_LINE_FILTERS_EXPR} ${VAR_FIELDS_EXPR} [$__auto])) by (${streamSelectorName})`;
}
8 changes: 4 additions & 4 deletions src/services/variables.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,11 @@ export const VAR_LINE_FILTER_EXPR = '${lineFilterV2}';
// The new multi value line filter (ad-hoc variable)
export const VAR_LINE_FILTERS = 'lineFilters';
export const VAR_LINE_FILTERS_EXPR = '${lineFilters}';
export const LOG_STREAM_SELECTOR_EXPR = `{${VAR_LABELS_EXPR}} ${VAR_LEVELS_EXPR} ${VAR_METADATA_EXPR} ${VAR_PATTERNS_EXPR} ${VAR_LINE_FILTER_EXPR} ${VAR_LINE_FILTERS_EXPR} ${VAR_LOGS_FORMAT_EXPR} ${VAR_FIELDS_EXPR}`;
export const LOG_STREAM_SELECTOR_EXPR = `{${VAR_LABELS_EXPR}} ${VAR_LEVELS_EXPR} ${VAR_METADATA_EXPR} ${VAR_PATTERNS_EXPR} ${VAR_LINE_FILTERS_EXPR} ${VAR_LOGS_FORMAT_EXPR} ${VAR_FIELDS_EXPR}`;
// Same as the LOG_STREAM_SELECTOR_EXPR, but without the fields as they will need to be built manually to exclude the current filter value
export const DETECTED_FIELD_VALUES_EXPR = `{${VAR_LABELS_EXPR}} ${VAR_LEVELS_EXPR} ${VAR_METADATA_EXPR} ${VAR_PATTERNS_EXPR} ${VAR_LINE_FILTER_EXPR} ${VAR_LINE_FILTERS_EXPR} ${VAR_LOGS_FORMAT_EXPR} ${PENDING_FIELDS_EXPR}`;
export const DETECTED_METADATA_VALUES_EXPR = `{${VAR_LABELS_EXPR}} ${VAR_LEVELS_EXPR} ${PENDING_FIELDS_EXPR} ${VAR_PATTERNS_EXPR} ${VAR_LINE_FILTER_EXPR} ${VAR_LINE_FILTERS_EXPR} ${VAR_LOGS_FORMAT_EXPR} ${VAR_FIELDS_EXPR}`;
export const DETECTED_LEVELS_VALUES_EXPR = `{${VAR_LABELS_EXPR}} ${PENDING_FIELDS_EXPR} ${VAR_METADATA_EXPR} ${VAR_PATTERNS_EXPR} ${VAR_LINE_FILTER_EXPR} ${VAR_LINE_FILTERS_EXPR} ${VAR_LOGS_FORMAT_EXPR} ${VAR_FIELDS_EXPR}`;
export const DETECTED_FIELD_VALUES_EXPR = `{${VAR_LABELS_EXPR}} ${VAR_LEVELS_EXPR} ${VAR_METADATA_EXPR} ${VAR_PATTERNS_EXPR} ${VAR_LINE_FILTERS_EXPR} ${VAR_LOGS_FORMAT_EXPR} ${PENDING_FIELDS_EXPR}`;
export const DETECTED_METADATA_VALUES_EXPR = `{${VAR_LABELS_EXPR}} ${VAR_LEVELS_EXPR} ${PENDING_FIELDS_EXPR} ${VAR_PATTERNS_EXPR} ${VAR_LINE_FILTERS_EXPR} ${VAR_LOGS_FORMAT_EXPR} ${VAR_FIELDS_EXPR}`;
export const DETECTED_LEVELS_VALUES_EXPR = `{${VAR_LABELS_EXPR}} ${PENDING_FIELDS_EXPR} ${VAR_METADATA_EXPR} ${VAR_PATTERNS_EXPR} ${VAR_LINE_FILTERS_EXPR} ${VAR_LOGS_FORMAT_EXPR} ${VAR_FIELDS_EXPR}`;
export const PATTERNS_SAMPLE_SELECTOR_EXPR = `{${VAR_LABELS_EXPR}} ${VAR_METADATA_EXPR} ${VAR_PATTERNS_EXPR} ${VAR_LOGS_FORMAT_EXPR}`;
export const EXPLORATION_DS = { uid: VAR_DATASOURCE_EXPR };
export const ALL_VARIABLE_VALUE = '$__all';
Expand Down
Loading

0 comments on commit 0c1b45d

Please sign in to comment.