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

fix(Labels): Fix "Discarded by user" error in the UI #110

Merged
merged 6 commits into from
Aug 6, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ export class SceneErrorState extends SceneObjectBase<SceneErrorStateState> {
public static Component = ({ model }: SceneComponentProps<SceneErrorState>) => {
const { message } = model.useState();
return (
<Alert title="Query error" severity="error">
<Alert title="Query error!" severity="error">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cosmetics...

{message}
</Alert>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,16 +93,14 @@ export class GroupByVariable extends QueryVariable {
console.error(error);

return (
<>
<Field label="Group by labels">
<div className={styles.groupByErrorContainer}>
<Tooltip theme="error" content={error.toString()}>
<Icon className={styles.iconError} name="exclamation-triangle" size="xl" />
</Tooltip>
<RefreshPicker noIntervalPicker onRefresh={model.update} isOnCanvas={false} onIntervalChanged={noOp} />
</div>
</Field>
</>
<Field label="Group by labels">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cosmetics as well

<div className={styles.groupByErrorContainer}>
<Tooltip theme="error" content={error.toString()}>
<Icon className={styles.iconError} name="exclamation-triangle" size="xl" />
</Tooltip>
<RefreshPicker noIntervalPicker onRefresh={model.update} isOnCanvas={false} onIntervalChanged={noOp} />
</div>
</Field>
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,37 +9,30 @@ import {
} from '@grafana/data';
import { RuntimeDataSource, sceneGraph } from '@grafana/scenes';
import { isPrivateLabel } from '@shared/components/QueryBuilder/domain/helpers/isPrivateLabel';
import { labelsRepository } from '@shared/infrastructure/labels/labelsRepository';
import { LabelsRepository } from '@shared/infrastructure/labels/labelsRepository';
import { MemoryCacheClient } from '@shared/infrastructure/MemoryCacheClient';

import { GroupByVariable } from '../../domain/variables/GroupByVariable/GroupByVariable';
import { computeRoundedTimeRange } from '../../helpers/computeRoundedTimeRange';
import { PYROSCOPE_LABELS_DATA_SOURCE } from '../pyroscope-data-sources';
import { DataSourceProxyClientBuilder } from '../series/http/DataSourceProxyClientBuilder';
import { LabelsApiClient } from './http/LabelsApiClient';

// we instantiate a new repository here to prevent unwanted interaction with the QueryBuilder
// indeed, when entering the "idle" state, the QueryBuilder state machine will call cancelAllLoad()
// that will abort any ApiClient in-flight request
const labelsRepository = new LabelsRepository({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fix

apiClient: new LabelsApiClient({ dataSourceUid: '' }),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the uid forced to be empty here, compared to

export const labelsRepository = new LabelsRepository({
apiClient: new LabelsApiClient(),
cacheClient: new MemoryCacheClient(),
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because they are different class of API clients. This one, used by LabelsDataSource inherits from DataSourceProxyClient, which requires a data source UID when instantiated. This allows us to switch the data source in runtime when the user selects different data source in the UI.

The other one inherits from ApiClient, which determines the data source UID only when the user lands on the page for the 1st time. It's used by the (legacy) Comparison page where we don't need to switch the data source in runtime.

It's confusing, but it's temporal. Once we migrate the Comparison page, we'll only need the client inheriting from DataSourceProxyClient.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for explaining, that makes sense 👍

cacheClient: new MemoryCacheClient(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth reusing the cache between the two instances of this?

export const labelsRepository = new LabelsRepository({
apiClient: new LabelsApiClient(),
cacheClient: new MemoryCacheClient(),
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. By making the change, I actually realised that this fix is not the best we can do. I've just commited the new fix.

});

export class LabelsDataSource extends RuntimeDataSource {
static MAX_TIMESERIES_LABEL_VALUES = 10;

constructor() {
super(PYROSCOPE_LABELS_DATA_SOURCE.type, PYROSCOPE_LABELS_DATA_SOURCE.uid);
}

async fetchLabels(dataSourceUid: string, query: string, from: number, to: number) {
const labelsApiClient = DataSourceProxyClientBuilder.build(dataSourceUid, LabelsApiClient) as LabelsApiClient;

labelsRepository.setApiClient(labelsApiClient);

return labelsRepository.listLabels({ query, from, to });
}

async fetchLabelValues(dataSourceUid: string, query: string, from: number, to: number, label: string) {
const labelsApiClient = DataSourceProxyClientBuilder.build(dataSourceUid, LabelsApiClient) as LabelsApiClient;

labelsRepository.setApiClient(labelsApiClient);

return labelsRepository.listLabelValues({ query, from, to, label });
}

async query(): Promise<DataQueryResponse> {
return {
state: LoadingState.Done,
Expand All @@ -60,18 +53,40 @@ export class LabelsDataSource extends RuntimeDataSource {
};
}

async metricFindQuery(query: string, options: LegacyMetricFindQueryOptions): Promise<MetricFindValue[]> {
getParams(options: LegacyMetricFindQueryOptions) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some refactoring.

const { scopedVars, range } = options;
const sceneObject = scopedVars?.__sceneObject?.value as GroupByVariable;

const dataSourceUid = sceneGraph.interpolate(sceneObject, '$dataSource');
const serviceName = sceneGraph.interpolate(sceneObject, '$serviceName');
const profileMetricId = sceneGraph.interpolate(sceneObject, '$profileMetricId');

// we could interpolate ad hoc filters, but the Labels exploration type would reload all labels each time they are modified
// const filters = sceneGraph.interpolate(sceneObject, '$filters');
// const pyroscopeQuery = `${profileMetricId}{service_name="${serviceName}",${filters}}`;
const query = `${profileMetricId}{service_name="${serviceName}"}`;

const { from, to } = computeRoundedTimeRange(range as TimeRange);

return {
dataSourceUid,
serviceName,
profileMetricId,
query,
from,
to,
};
}

async metricFindQuery(_: string, options: LegacyMetricFindQueryOptions): Promise<MetricFindValue[]> {
const sceneObject = options.scopedVars?.__sceneObject?.value as GroupByVariable;

// save bandwidth
if (!sceneObject.isActive) {
return [];
}

const dataSourceUid = sceneGraph.interpolate(sceneObject, '$dataSource');
const serviceName = sceneGraph.interpolate(sceneObject, '$serviceName');
const profileMetricId = sceneGraph.interpolate(sceneObject, '$profileMetricId');
const { dataSourceUid, serviceName, profileMetricId, query, from, to } = this.getParams(options);

if (!serviceName || !profileMetricId) {
console.warn(
Expand All @@ -82,22 +97,19 @@ export class LabelsDataSource extends RuntimeDataSource {
return [];
}

// we could interpolate ad hoc filters, but the Service labels exploration type would reload all labels each time they are modified
// const filters = sceneGraph.interpolate(sceneObject, '$filters');
// const pyroscopeQuery = `${profileMetricId}{service_name="${serviceName}",${filters}}`;
const pyroscopeQuery = `${profileMetricId}{service_name="${serviceName}"}`;

const { from, to } = computeRoundedTimeRange(range as TimeRange);
labelsRepository.setApiClient(
DataSourceProxyClientBuilder.build(dataSourceUid, LabelsApiClient) as LabelsApiClient
);

const labels = await this.fetchLabels(dataSourceUid, pyroscopeQuery, from, to);
const labels = await labelsRepository.listLabels({ query, from, to });

const sortedLabelsWithCounts = (
await Promise.all(
labels
.filter(({ value }) => !isPrivateLabel(value))
.sort((a, b) => a.label.localeCompare(b.label))
.map(async ({ value }) => {
const values = (await this.fetchLabelValues(dataSourceUid, pyroscopeQuery, from, to, value)).map(
const values = (await labelsRepository.listLabelValues({ query, from, to, label: value })).map(
({ value }) => value
);
const count = values.length;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ export function filtersToQuery(query: string, filters: Filters) {
.map((filter) => {
const { attribute, operator, value } = filter as CompleteFilter;

if (operator.value === OperatorKind.in) {
return `${attribute.value}=~"${value.value}"`;
}
// if (operator.value === OperatorKind.in) {
Copy link
Contributor Author

@grafakus grafakus Aug 5, 2024

Choose a reason for hiding this comment

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

Not strictly related to this PR, but as we don't support the in operator, we shouldn't let this code being executed.

// return `${attribute.value}=~"${value.value}"`;
// }

// TODO: use "attribute-operator" FilterKind? We still set a value for these filters that we could use here.
if (operator.value === OperatorKind['is-empty']) {
Expand Down
34 changes: 17 additions & 17 deletions src/shared/components/QueryBuilder/domain/helpers/queryToFilters.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,23 +48,23 @@ export function queryToFilters(query: string): Filters {
};
}

const shouldChangeToInOperator = operator === OperatorKind['=~'] && value.includes('|');
if (shouldChangeToInOperator) {
return {
id: nanoid(10),
type: FilterKind['attribute-operator-value'],
active: true,
attribute: { value: attribute, label: attribute },
operator: { value: OperatorKind.in, label: OperatorKind.in },
value: {
value: value,
label: value
.split('|')
.map((v) => v.trim())
.join(', '),
},
};
}
// const shouldChangeToInOperator = operator === OperatorKind['=~'] && value.includes('|');
grafakus marked this conversation as resolved.
Show resolved Hide resolved
// if (shouldChangeToInOperator) {
// return {
// id: nanoid(10),
// type: FilterKind['attribute-operator-value'],
// active: true,
// attribute: { value: attribute, label: attribute },
// operator: { value: OperatorKind.in, label: OperatorKind.in },
// value: {
// value: value,
// label: value
// .split('|')
// .map((v) => v.trim())
// .join(', '),
// },
// };
// }

return {
id: nanoid(10),
Expand Down
2 changes: 1 addition & 1 deletion src/shared/components/QueryBuilder/domain/stateMachine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ export function buildStateMachine(inputParams: InputParams) {
...defaultContext,
inputParams,
query,
// See changeInputParams() in src/overrides/components/TagsBar/QueryBuilder/domain/actions.ts
// See changeInputParams() in domain/actions.ts
filters: queryToFilters(query),
};

Expand Down
2 changes: 1 addition & 1 deletion src/shared/infrastructure/labels/labelsRepository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ type ListLabelValuesOptions = ListLabelsOptions & {
label: string;
};

class LabelsRepository extends AbstractRepository<LabelsApiClient, MemoryCacheClient> {
export class LabelsRepository extends AbstractRepository<LabelsApiClient, MemoryCacheClient> {
cacheClient: MemoryCacheClient;

static isNotMetaLabelOrServiceName = (label: string) => !/^(__.+__|service_name)$/.test(label);
Expand Down
Loading