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

grafakus · 2024-08-05T10:23:24Z

✨ Description

Related issue(s): -

This PR fixes an error that can occur when changing the data source or clicking on the main refresh button in the UI:

Additionally, this PR also fixes a potential issue with the query builder, which does not update its API client whenever the data source changes.

📖 Summary of the changes

The error comes from an unwanted interaction between the query builder and LabelsDataSource where the API request made by LabelsDataSource to fetch label names is cancelled by the query builder state machine. This happens because the both share the same instance of the LabelsRepository class, which in turn uses the same API client.

The scenario is the following:

Whenever the user changes the data source:

GroupByVariable gets its options updated via LabelsDataSource
In LabelsDataSource, LabelsRepository sets the API client corresponding to the new data source
the query builder state machine receives new input parameters, which in turn, causes the machine to transition to its idle state
when entering this state, all in-flight requests are cancelled (see QueryBuilder/domain/actions.ts)
because the same API client is used by LabelsRepository, this throws an abort error that bubbles up to the "Group by" UI

The solution is to make sure distinct instances of the API client are created by the query builder and by LabelsDataSource.

See the diff tab for additional specific comments.

🧪 How to test?

Manually, after checking out this PR's branch

grafakus · 2024-08-05T10:24:15Z

...erView/components/SceneByVariableRepeaterGrid/components/SceneErrorState/SceneErrorState.tsx

@@ -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">


Cosmetics...

grafakus · 2024-08-05T10:24:24Z

src/pages/ProfilesExplorerView/domain/variables/GroupByVariable/GroupByVariable.tsx

-            </div>
-          </Field>
-        </>
+        <Field label="Group by labels">


Cosmetics as well

grafakus · 2024-08-05T10:26:19Z

src/shared/components/QueryBuilder/domain/helpers/filtersToQuery.ts

-      if (operator.value === OperatorKind.in) {
-        return `${attribute.value}=~"${value.value}"`;
-      }
+      // if (operator.value === OperatorKind.in) {


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

grafakus · 2024-08-05T10:32:38Z

src/pages/ProfilesExplorerView/infrastructure/labels/LabelsDataSource.ts

+// 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({


github-actions · 2024-08-05T10:34:11Z

Unit test coverage

Lines	Statements	Branches	Functions
	12.04% (464/3853)	9.35% (134/1432)	9.11% (107/1174)

src/shared/components/QueryBuilder/domain/helpers/queryToFilters.ts

grafakus · 2024-08-05T10:34:40Z

src/shared/components/QueryBuilder/domain/helpers/__tests__/filtersToQuery.spec.ts

@@ -60,23 +60,23 @@ const spanNameFilter = {
  },
 };

-const podIdFilter = {


See comments below

simonswine

Almost LGTM

A couple of smaller questions/suggestion

simonswine · 2024-08-05T10:53:15Z

src/pages/ProfilesExplorerView/infrastructure/labels/LabelsDataSource.ts

+// 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({
+  apiClient: new LabelsApiClient({ dataSourceUid: '' }),


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

explore-profiles/src/shared/infrastructure/labels/labelsRepository.ts

Lines 115 to 118 in 4ee541a

export const labelsRepository = new LabelsRepository({

apiClient: new LabelsApiClient(),

cacheClient: new MemoryCacheClient(),

});

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.

Thanks for explaining, that makes sense 👍

src/shared/components/QueryBuilder/domain/helpers/queryToFilters.ts

simonswine · 2024-08-05T10:59:46Z

src/pages/ProfilesExplorerView/infrastructure/labels/LabelsDataSource.ts

+// that will abort any ApiClient in-flight request
+const labelsRepository = new LabelsRepository({
+  apiClient: new LabelsApiClient({ dataSourceUid: '' }),
+  cacheClient: new MemoryCacheClient(),


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

explore-profiles/src/shared/infrastructure/labels/labelsRepository.ts

Lines 115 to 118 in 4ee541a

export const labelsRepository = new LabelsRepository({

apiClient: new LabelsApiClient(),

cacheClient: new MemoryCacheClient(),

});

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.

grafakus · 2024-08-05T12:39:24Z

src/pages/ProfilesExplorerView/infrastructure/labels/LabelsDataSource.ts

-    const pyroscopeQuery = `${profileMetricId}{service_name="${serviceName}"}`;
-
-    const { from, to } = computeRoundedTimeRange(range as TimeRange);
+    labelsRepository.setApiClient(new LabelsApiClient({ dataSourceUid }));


The fix. We don't cache the API client anymore with DataSourceProxyClientBuilder, so that when the query builder aborts a request, it will do it via its own API client.

We continue using the same instance of LabelsRepository to be able to benefit from a shared cache.

grafakus · 2024-08-05T12:39:40Z

src/pages/ProfilesExplorerView/infrastructure/labels/LabelsDataSource.ts

@@ -60,18 +43,40 @@ export class LabelsDataSource extends RuntimeDataSource {
    };
  }

-  async metricFindQuery(query: string, options: LegacyMetricFindQueryOptions): Promise<MetricFindValue[]> {
+  getParams(options: LegacyMetricFindQueryOptions) {


Some refactoring.

grafakus · 2024-08-05T12:41:12Z

src/shared/components/QueryBuilder/domain/actions.ts

+    // TODO: remove this condition after migrating the legacy comparison pages to Scenes
+    // because dataSourceUid will always be provided
+    if (event.data.dataSourceUid) {
+      labelsRepository.setApiClient(new LabelsApiClient({ dataSourceUid: event.data.dataSourceUid }));


The 2nd fix, we need to update the API client every time the data source changes.

simonswine

LGTM

Thanks for helping me understand this. I think this is ready to go.

fix(LabelsDataSource): Use own LabelsRepository instance

36a68a9

github-actions bot added the fix label Aug 5, 2024

grafakus commented Aug 5, 2024

View reviewed changes

grafakus requested review from simonswine and bryanhuhta August 5, 2024 10:26

fix(*): Fix query builder UT

a300914

grafakus commented Aug 5, 2024

View reviewed changes

src/shared/components/QueryBuilder/domain/helpers/queryToFilters.ts Show resolved Hide resolved

grafakus commented Aug 5, 2024

View reviewed changes

Merge branch 'main' into fix/labels-error-when-changing-ds

b7fbd22

simonswine reviewed Aug 5, 2024

View reviewed changes

chore: Clarify why some code is commented

7320b51

grafakus marked this pull request as draft August 5, 2024 12:21

grafakus added 2 commits August 5, 2024 14:25

fix(LabelsDataSource): remove DataSourceProxyClientBuilder

d0d2e7a

fix(QueryBuilder): Pass data source UID

ee57f61

grafakus marked this pull request as ready for review August 5, 2024 12:36

grafakus changed the title ~~fix(LabelsDataSource): Use a new independent LabelsRepository instance~~ fix(LabelsDataSource): Fix "Discarded by user" error in the UI Aug 5, 2024

grafakus changed the title ~~fix(LabelsDataSource): Fix "Discarded by user" error in the UI~~ fix(Labels): Fix "Discarded by user" error in the UI Aug 5, 2024

grafakus commented Aug 5, 2024

View reviewed changes

grafakus requested a review from simonswine August 5, 2024 12:50

simonswine approved these changes Aug 5, 2024

View reviewed changes

grafakus merged commit 2e9baab into main Aug 6, 2024
11 of 12 checks passed

grafakus deleted the fix/labels-error-when-changing-ds branch August 6, 2024 08:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

grafakus commented Aug 5, 2024 •

edited

Loading

grafakus Aug 5, 2024

grafakus Aug 5, 2024

grafakus Aug 5, 2024 •

edited

Loading

grafakus Aug 5, 2024

github-actions bot commented Aug 5, 2024 •

edited

Loading

grafakus Aug 5, 2024

simonswine left a comment

simonswine Aug 5, 2024

grafakus Aug 5, 2024

simonswine Aug 5, 2024

simonswine Aug 5, 2024

grafakus Aug 5, 2024

grafakus Aug 5, 2024 •

edited

Loading

grafakus Aug 5, 2024

grafakus Aug 5, 2024 •

edited

Loading

simonswine left a comment

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

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

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

Conversation

grafakus commented Aug 5, 2024 • edited Loading

✨ Description

📖 Summary of the changes

🧪 How to test?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grafakus Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 5, 2024 • edited Loading

Unit test coverage

Choose a reason for hiding this comment

simonswine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grafakus Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grafakus Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

simonswine left a comment

Choose a reason for hiding this comment

grafakus commented Aug 5, 2024 •

edited

Loading

grafakus Aug 5, 2024 •

edited

Loading

github-actions bot commented Aug 5, 2024 •

edited

Loading

grafakus Aug 5, 2024 •

edited

Loading

grafakus Aug 5, 2024 •

edited

Loading