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.

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

Jump to bottom

Telemetry #169

Merged
merged 5 commits into from
Feb 6, 2024
Merged

Telemetry #169

merged 5 commits into from
Feb 6, 2024

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Jan 29, 2024 •
edited
Loading

Fixes partially #35

@feloy feloy force-pushed the feat-35/telemetry branch 6 times, most recently from 6411f97 to 79cf077 Compare January 29, 2024 15:30
@feloy feloy changed the title [wip] Telemetry Telemetry Jan 29, 2024
@feloy feloy force-pushed the feat-35/telemetry branch 4 times, most recently from 5a6dc4e to 242d352 Compare January 30, 2024 09:42
lstocchi
lstocchi reviewed Jan 30, 2024
View reviewed changes
Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

Not going to approve as it's better that Jeff or Florent or Stevan look at it.
LGTM, i'm just unsure about the telemetry event names. I remember that we needed to use the function name 🤔 So instead of using recipe.pull we should use the name of the function we are into, but i'll wait for the comment of some expert

axel7083
axel7083 reviewed Jan 30, 2024
View reviewed changes
packages/shared/src/StudioAPI.ts Outdated
@@ -25,4 +26,9 @@ export abstract class StudioAPI {
abstract askPlayground(modelId: string, prompt: string): Promise<number>;
abstract getPlaygroundQueriesState(): Promise<QueryState[]>;
abstract getPlaygroundsState(): Promise<PlaygroundState[]>;

// eslint-disable-next-line @typescript-eslint/no-explicit-any
abstract telemetryLogUsage(eventName: string, data?: Record<string, any | TelemetryTrustedValue>): Promise<void>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we use unknown ?

@feloy feloy force-pushed the feat-35/telemetry branch 3 times, most recently from a78db26 to 893c7ef Compare January 31, 2024 06:40
jeffmaury
jeffmaury reviewed Feb 1, 2024
View reviewed changes
Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

Also would be good to have timing info for the pull operation

packages/backend/src/managers/applicationManager.spec.ts Outdated
{
logUsage: mocks.logUsageMock,
logError: mocks.logErrorMock,
} as unknown as TelemetryLogger,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we have a shared variable to represent the telemetry logger ?

packages/backend/src/managers/modelsManager.ts Outdated
@@ -173,6 +175,7 @@ export class ModelsManager {
});

try {
this.telemetry.logUsage('model.download', { 'model.id': model.id });
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to have a single event: here we will have 2 events in case of error

packages/backend/src/managers/modelsManager.ts Outdated
@@ -184,6 +187,7 @@ export class ModelsManager {
'model-pulling': model.id,
},
});
this.telemetry.logError('model.download', { message: 'error downloading model', error: e });
Copy link
Contributor

Choose a reason for hiding this comment

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

model.id is missing.

packages/backend/src/managers/modelsManager.ts Outdated
@@ -173,6 +175,7 @@ export class ModelsManager {
});

try {
this.telemetry.logUsage('model.download', { 'model.id': model.id });
Copy link
Contributor

Choose a reason for hiding this comment

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

Also would be good to have execution time for the action (download)

packages/backend/src/managers/playground.ts Outdated
@@ -107,6 +109,7 @@ export class PlayGroundManager {
const connection = findFirstProvider();
if (!connection) {
this.setPlaygroundStatus(modelId, 'error');
this.telemetry.logError('playground.start', { message: 'unable to find an engine to start playground' });
Copy link
Contributor

Choose a reason for hiding this comment

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

add model.id

packages/backend/src/studio-api-impl.ts Outdated
@@ -71,6 +72,8 @@ export class StudioApiImpl implements StudioAPI {
const recipe = this.catalogManager.getRecipes().find(recipe => recipe.id === recipeId);
if (!recipe) throw new Error('Not found');

await this.telemetryLogUsage('recipe.pull', { 'recipe.id': recipe.id, 'recipe.name': recipe.name });
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure the telemetry event should be located here which is a gateway or we need to better handle errors

@feloy feloy requested a review from a team as a code owner February 1, 2024 10:28
@feloy feloy changed the title Telemetry [wip] Telemetry Feb 1, 2024
@feloy feloy force-pushed the feat-35/telemetry branch 2 times, most recently from 47805b5 to 1533275 Compare February 1, 2024 13:41
@feloy feloy requested a review from jeffmaury February 1, 2024 13:47
@feloy feloy changed the title [wip] Telemetry Telemetry Feb 1, 2024
@feloy feloy force-pushed the feat-35/telemetry branch from 1533275 to 853e2ee Compare February 1, 2024 15:38
jeffmaury
jeffmaury reviewed Feb 5, 2024
View reviewed changes
packages/backend/src/managers/applicationManager.ts Outdated
@@ -157,6 +161,7 @@ export class ApplicationManager {
await timeout(5000);
await this.restartContainerWhenModelServiceIsUp(engineId, modelServiceEndpoint, container).catch(
(error: unknown) => {
this.telemetry.logError('recipe.pull', { message: 'error monitoring endpoint', error: error });
Copy link
Contributor

Choose a reason for hiding this comment

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

Event is not uniform here recipe id, recipe name and duration are not sent. I'm wondering if we should have a single point for sending this event (pullApplication) and handle exceptions there

@feloy feloy force-pushed the feat-35/telemetry branch from 853e2ee to 37d8372 Compare February 5, 2024 08:30
@feloy feloy force-pushed the feat-35/telemetry branch from 37d8372 to 0f048ce Compare February 5, 2024 08:32
@feloy feloy requested a review from jeffmaury February 5, 2024 08:54
@feloy feloy force-pushed the feat-35/telemetry branch from 6889845 to fe979fa Compare February 5, 2024 15:45
jeffmaury
jeffmaury reviewed Feb 5, 2024
View reviewed changes
packages/backend/src/managers/modelsManager.ts Show resolved Hide resolved
packages/backend/src/managers/playground.ts Outdated
@@ -237,6 +247,7 @@ export class PlayGroundManager {
modelId,
});

this.telemetry.logUsage('playground.start', { 'model.id': modelId });
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to have the time to start playground as well

packages/backend/src/managers/playground.ts Outdated
@@ -294,7 +312,7 @@ export class PlayGroundManager {
this.sendQueriesState();
}
})().catch((err: unknown) => console.warn(`Error while reading streamed response for model ${modelInfo.id}`, err));

this.telemetry.logUsage('playground.ask', { 'model.id': modelInfo.id });
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should also record the execution time for a request but that could be a follow up PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added an entry to #35 to track this on a follow up PR

@feloy feloy force-pushed the feat-35/telemetry branch 2 times, most recently from 8b5a1b2 to 1c420ea Compare February 5, 2024 17:59
@feloy feloy requested a review from jeffmaury February 5, 2024 18:33
jeffmaury
jeffmaury reviewed Feb 6, 2024
View reviewed changes
Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

Execution time is missing for stopPlayground

@feloy feloy force-pushed the feat-35/telemetry branch from 15869e2 to 84c96e7 Compare February 6, 2024 09:09
@feloy feloy force-pushed the feat-35/telemetry branch from 84c96e7 to 97e3915 Compare February 6, 2024 09:16
@feloy feloy requested a review from jeffmaury February 6, 2024 09:20
jeffmaury
jeffmaury approved these changes Feb 6, 2024
View reviewed changes
Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

LGTM

@feloy feloy merged commit 2f4b3a7 into main Feb 6, 2024
3 checks passed
@feloy feloy deleted the feat-35/telemetry branch February 6, 2024 13:01
@feloy feloy mentioned this pull request Feb 12, 2024
Closed
1 task
mhdawson pushed a commit to mhdawson/podman-desktop-extension-ai-lab that referenced this pull request Nov 22, 2024
@sallyom
Merge pull request containers#169 from sallyom/update-make-bootc
2122947 
update make bootc for chatbot, summarizer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lstocchi lstocchi lstocchi left review comments

@axel7083 axel7083 axel7083 left review comments

@jeffmaury jeffmaury jeffmaury approved these changes

@benoitf benoitf Awaiting requested review from benoitf benoitf is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@feloy @jeffmaury @axel7083 @lstocchi