[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]

We should probably separate the types of the context and the type that is sent with the submitMetrics.

[shahrokni]

Actually the parameter of the submit function is a subset of the main interface.
So, it should be fine no?
So, as you can see the extra stuff is dropped from the param.

type SubmitMetrics = (
  param: Omit<UsageMetrics, 'setRenderDurationMs' | 'setRenderErrorCount' | 'setPendingQueries'>
) => Promise<void>;

[jgbernalp]

The name does not describe what the interface holds, one is the UsageMetricsContext, the other one is the actual UsageMetrics that is sent to the backend.

[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]

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;
}