[BREAKINGCHANGE] remove fetch from usage metrics

[shahrokni]

This PR removes the fetch (from core) dependency in plugin-system\src\runtime\UsageMetricsProvider.tsx
This is an IOC (inversion of control) kind of change which gives the consumer the responsibility of Submitting Metrics as this should not be the concern of the plugin-system. From the Perses/Perses perspective, this responsibility will reside in UI, probably somewhere around ui\app\src\views\projects\dashboards\HelperDashboardView.tsx

The new added prop (submitMetric) is optional, so this is not a breaking change. From Perses/Perses we need to provide the submit function

This change also includes a bug fix. Apparently, there is a direct mutation of the context which should be fixed by this PR.

Finally there is a change in test files. Please read the comment about the dependency

/* All GlobalDatasourceResource, DatasourceResource, etc ... 
   have been used for the test purpose only, creating a wrong dependency to the core!
   We either move this test (somehow) to the Perses/UI or we need to duplicate the types
*/

Checklist

• Pull request has a descriptive title and context useful to a reviewer.
• Pull request title follows the [<catalog_entry>] <commit message> naming convention using one of the
  following catalog_entry values: FEATURE, ENHANCEMENT, BUGFIX, BREAKINGCHANGE, DOC,IGNORE.
• All commits have DCO signoffs.

UI Changes

• Changes that impact the UI include screenshots and/or screencasts of the relevant changes.
• Code follows the UI guidelines.
• E2E tests are stable and unlikely to be flaky.
  See e2e docs for more details. Common issues include:
  • Is the data inconsistent? You need to mock API requests.
  • Does the time change? You need to use consistent time values or mock time utilities.
  • Does it have loading states? You need to wait for loading to complete.

[jgbernalp]

I guess is coming after this is merged. But we need to adjust Perses UI to submit the metrics.

[shahrokni]

Is the comment suggesting I need to change something here, or it is only an announcment. Because =>
Yes, we must to provide the submitMetrics from the Perses/UI side.

[jgbernalp]

We should have at least an issue to track this should be added, otherwise usage metrics won't be stored leading to data loss

[shahrokni]

We should have at least an issue to track this should be added, otherwise usage metrics won't be stored leading to data loss

Let's lift the optional ? and then make it BREAKINGCHANGE. Inherently it is BREAKINGCHANGE.
WDYT?

[jgbernalp]

yes but I think we still need a GitHub issue to track that the perses app is adjusted based on this breaking change, otherwise this could get forgotten and users installing a new version will see is no longer tracking usage...

[shahrokni]

Done => perses/perses#4046

[jgbernalp]

This is breaking the plugins that use this interface. I don't think the interface should be changed like this. A better approach would be to create a fetchProvider which plugins or components can use

[shahrokni]

So, as you know we are trying to drop the fetch-core dependency. Where do you want to place the fetchProvider? Such a provider will be placed in the shared, meaning, again there will be the same dependency but somewhere else. Unless, we move the entire logic of the fetch into that provider under the plugin-system,

Again, maybe it could have its own small package...
This is becoming a dilemma
It should either move to plugin-system or have its own package.

/* FROM CORE =to=> plugin-system or a dedicated package */
export async function fetch(...args: Parameters<typeof globalThis.fetch>): Promise<Response> {
  const response = await globalThis.fetch(...args);
  if (response.ok === false) {
    const contentType = response.headers.get('content-type');
    if (contentType?.includes('application/json')) {
      const json = await response.clone().json();
      if (json.error) {
        throw new UserFriendlyError(json.error, response.status);
      }
      if (json.message) {
        throw new UserFriendlyError(json.message, response.status);
      }
    }

    const text = await response.clone().text();
    if (text) {
      throw new UserFriendlyError(text, response.status);
    }
    throw new FetchError(response);
  }
  return response;
}

[jgbernalp]

it probably has to be in the new api package. But it will provide a default implementation like above, then you can use a hook from the plugins to use it. And the provider can accept a custom globalThis.fetch

[shahrokni]

@Nexucis
It seems inevitably there will be a small package!
This will contain the fetch and the following types.

@jgbernalp We also need to find a relevant name for the new package. @Nexucis believes api might be vague.

import { HTTPProxy } from './http-proxy';
export type RequestHeaders = Record<string, string>;
export interface HTTPDatasourceSpec {
    directUrl?: string;
    proxy?: HTTPProxy;
}

[jgbernalp]

Do we need to remove this type?

[shahrokni]

It should be removed. Please take a look at this file. This file is providing mock data (data source provider) for the test purpose only, and to do so, it has made a dependency to GlobalDatasourceResource (from core)
(The final goal is to eliminate the core dependency)

To remove the dependency we can simply remove the type and since the linter does not complain we are good to go. However, whether keeping this test here or not is another question. I think this test should be moved to the app with the new outline.

From slack (Nexucis): my point was more that everything that requires a knowledge about how the resources are managed in Perses should remain in the app.
For example, the notion of GlobalDatasource or ProjectDatasource should be kept in the app and instead in the package that requires this notion, you have an interface to implement.

So, yes, tests are not exempt from this idea. It think it should be moved eventually.

[jgbernalp]

But it seems we are adding a lot of technical debt adding intermediate types instead of fixing the root cause which is to define a location for this *Resource types. Wouldn't be better to address that first, so we can cleanup pointing to the actual dependency and avoid to have to come back and cleanup again?

[shahrokni]

OK, Let's discuss this the following types proper locations first.
These are coming from Core and do not belong to any packages that we have under shared.
Besides, the idea of a new package is in jeopardy! Please take a look at the slack conversation I created on Friday.
So, I am not really sure what we are going to do with them.

export interface GlobalDatasourceResource {
  kind: 'GlobalDatasource';
  metadata: Metadata;
  spec: DatasourceSpec;
}

/**
 * A Datasource resource, that belongs to a project.
 */
export interface DatasourceResource {
  kind: 'Datasource';
  metadata: ProjectMetadata;
  spec: DatasourceSpec;
}

export type Datasource = DatasourceResource | GlobalDatasourceResource;

[shahrokni]

@jgbernalp
Does that make sense to create a generic interface in the dashboards called Datasource?

Datasource<T>

This could solve the problem for us. It complies with. What do you think?

my point was more that everything that requires a knowledge about how the resources are managed in Perses should remain in the app.
For example, the notion of GlobalDatasource or ProjectDatasource should be kept in the app and instead in the package that requires this notion, you have an interface to implement.

But, still we need to deal with Metadata

[jgbernalp]

I don't think we need generics here, is one thing or the other, we don't need to add support for unknown type of dashboards. The union type should be enough. We can discuss later on the weekly regarding the new package consensus.