Skip to content

[BREAKINGCHANGE] remove fetch from usage metrics #124

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.

Sign up for GitHub

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

Open
shahrokni wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[BREAKINGCHANGE] remove fetch from usage metrics #124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 29 additions & 1 deletion dashboards/src/context/DatasourceStoreProvider.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,37 @@ import { DatasourceStoreProvider } from '@perses-dev/dashboards';
import { PropsWithChildren, ReactElement } from 'react';
import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
import { DashboardSpec, DatasourceSpec, UnknownSpec } from '@perses-dev/spec';
import { Datasource, DatasourceResource, GlobalDatasourceResource } from '@perses-dev/core';
import { DashboardResource } from '../model/DashboardResource';

/* TODO: 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
*/
interface ProjectMetadata extends Metadata {
project: string;
}

interface Metadata {
name: string;
createdAt?: string;
updatedAt?: string;
version?: number;
tags?: string[];
}

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

interface DatasourceResource {
kind: 'Datasource';
metadata: ProjectMetadata;
spec: DatasourceSpec;
}
type Datasource = DatasourceResource | GlobalDatasourceResource;

const PROJECT = 'perses';
const FAKE_PLUGIN_NAME = 'FakeDatasourcePlugin';

Expand Down
3 changes: 1 addition & 2 deletions dashboards/src/test/datasource-provider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,11 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import { GlobalDatasourceResource } from '@perses-dev/core';
import { DatasourceStoreProviderProps } from '../context';
import { getTestDashboard } from './dashboard-provider';

export const prometheusDemoUrl = 'https://prometheus.demo.prometheus.io';
export const prometheusDemo: GlobalDatasourceResource = {
export const prometheusDemo = {
Copy link
Copy Markdown
Contributor

@jgbernalp jgbernalp May 4, 2026

Choose a reason for hiding this comment

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

Do we need to remove this type?

Copy link
Copy Markdown
Contributor Author

@shahrokni shahrokni May 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@jgbernalp jgbernalp May 4, 2026

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

@shahrokni shahrokni May 4, 2026

Choose a reason for hiding this comment

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

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;

Copy link
Copy Markdown
Contributor Author

@shahrokni shahrokni May 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

@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

Copy link
Copy Markdown
Contributor

@jgbernalp jgbernalp May 4, 2026

Choose a reason for hiding this comment

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

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.

kind: 'GlobalDatasource',
metadata: {
name: 'PrometheusDemo',
Expand Down
4 changes: 4 additions & 0 deletions plugin-system/src/remote/remotePluginLoader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,10 @@ export function remotePluginLoader(options?: RemotePluginLoaderOptions): PluginL

return {
getInstalledPlugins: async (): Promise<PluginModuleResource[]> => {
/* TODO: This is already using the globalThis fetch and not the core one
So, we don't have any dependency to the core here.
The question is if the developer did that intentionally?!
*/
const pluginsResponse = await fetch(pluginsApiPath);

const plugins = await pluginsResponse.json();
Expand Down
105 changes: 71 additions & 34 deletions plugin-system/src/runtime/UsageMetricsProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,42 +11,83 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import { fetch } from '@perses-dev/core'; // TODO
import { QueryDefinition } from '@perses-dev/spec';
import { createContext, ReactElement, ReactNode, useContext } from 'react';
import { createContext, ReactElement, ReactNode, useContext, useEffect, useState } from 'react';

type QueryState = 'pending' | 'success' | 'error';

interface UsageMetrics {
export interface UsageMetrics {
project: string;
dashboard: string;
renderDurationMs: number;
renderErrorCount: number;
}

interface UsageMetricsContext {
project: string;
dashboard: string;
startRenderTime: number;
renderDurationMs: number;
setRenderDurationMs: React.Dispatch<React.SetStateAction<number>>;
renderErrorCount: number;
setRenderErrorCount: React.Dispatch<React.SetStateAction<number>>;
pendingQueries: Map<string, QueryState>;
setPendingQueries: React.Dispatch<React.SetStateAction<Map<string, QueryState>>>;
apiPrefix?: string;
submitMetrics: (metrics: UsageMetrics) => Promise<void>;
}

interface UsageMetricsProps {
export interface UsageMetricsProps {
project: string;
dashboard: string;
apiPrefix?: string;
children: ReactNode;
submitMetrics: (metrics: UsageMetrics) => Promise<void>;
}

interface UseUsageMetricsResults {
markQuery: (definition: QueryDefinition, state: QueryState) => void;
}

export const UsageMetricsContext = createContext<UsageMetrics | undefined>(undefined);
export const UsageMetricsContext = createContext<UsageMetricsContext | undefined>(undefined);

export const useUsageMetricsContext = (): UsageMetrics | undefined => {
export const useUsageMetricsContext = (): UsageMetricsContext | undefined => {
return useContext(UsageMetricsContext);
};

export const useUsageMetrics = (): UseUsageMetricsResults => {
const ctx = useUsageMetricsContext();

useEffect(() => {
if (!ctx) {
return;
}
const {
dashboard,
project,
renderErrorCount,
pendingQueries,
renderDurationMs,
startRenderTime,
submitMetrics,
setRenderDurationMs,
} = ctx;

/* This means no query has run yet, so it should return
The subsequent logic makes sense when a-some queries have been running
*/
if (!pendingQueries.size) {
return;
}

const allDone = [...pendingQueries.values()].every((p) => p !== 'pending');
if (renderDurationMs === 0 && allDone) {
const finalDuration = Date.now() - startRenderTime;
setRenderDurationMs(finalDuration);
submitMetrics({ dashboard, project, renderDurationMs, renderErrorCount });
}
}, [ctx]);

return {
markQuery: (definition: QueryDefinition, newState: QueryState): void => {
if (ctx === undefined) {
Expand All @@ -60,45 +101,41 @@ export const useUsageMetrics = (): UseUsageMetricsResults => {
}

if (ctx.pendingQueries.get(definitionKey) !== newState) {
ctx.pendingQueries.set(definitionKey, newState);
ctx.setPendingQueries((prev) => {
const map = new Map(prev);
map.set(definitionKey, newState);
return map;
});
if (newState === 'error') {
ctx.renderErrorCount += 1;
}

const allDone = [...ctx.pendingQueries.values()].every((p) => p !== 'pending');
if (ctx.renderDurationMs === 0 && allDone) {
ctx.renderDurationMs = Date.now() - ctx.startRenderTime;
submitMetrics(ctx);
ctx.setRenderErrorCount((prev) => prev + 1);
}
}
},
};
};

const submitMetrics = async (stats: UsageMetrics): Promise<void> => {
await fetch(`${stats.apiPrefix ?? ''}/api/v1/view`, {
method: 'POST',
headers: {
'Content-Type': 'application/json',
},
body: JSON.stringify({
project: stats.project,
dashboard: stats.dashboard,
render_time: stats.renderDurationMs / 1000,
render_errors: stats.renderErrorCount,
}),
});
};

export const UsageMetricsProvider = ({ apiPrefix, project, dashboard, children }: UsageMetricsProps): ReactElement => {
const ctx: UsageMetrics = {
export const UsageMetricsProvider = ({
apiPrefix,
project,
dashboard,
children,
submitMetrics,
Copy link
Copy Markdown
Contributor

@jgbernalp jgbernalp Apr 29, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@shahrokni shahrokni Apr 29, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@jgbernalp jgbernalp Apr 29, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@shahrokni shahrokni Apr 29, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor

@jgbernalp jgbernalp May 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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...

Copy link
Copy Markdown
Contributor Author

@shahrokni shahrokni May 4, 2026

Choose a reason for hiding this comment

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

Done => perses/perses#4046

}: UsageMetricsProps): ReactElement => {
const [renderDurationMs, setRenderDurationMs] = useState(0);
const [pendingQueries, setPendingQueries] = useState(new Map());
const [renderErrorCount, setRenderErrorCount] = useState(0);
const ctx: UsageMetricsContext = {
project: project,
dashboard: dashboard,
renderErrorCount: 0,
renderErrorCount,
startRenderTime: Date.now(),
renderDurationMs: 0,
pendingQueries: new Map(),
renderDurationMs,
setRenderDurationMs,
setPendingQueries,
setRenderErrorCount,
pendingQueries,
apiPrefix,
submitMetrics,
};

return <UsageMetricsContext.Provider value={ctx}>{children}</UsageMetricsContext.Provider>;
Expand Down
Loading