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

[Obs AI Assistant] Prevent error log when license is insufficient #181700

Merged
merged 2 commits into from
May 1, 2024
Merged

[Obs AI Assistant] Prevent error log when license is insufficient #181700

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f0c303d
[Obs AI Assistant] Prevent error log when license is insufficient
sorenlouv Apr 25, 2024
bbffc8b
Merge branch 'main' into prevent-error-log
kibanamachine Apr 30, 2024
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
18 changes: 9 additions & 9 deletions x-pack/plugins/observability_solution/observability_ai_assistant/server/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,22 +107,24 @@ export class ObservabilityAIAssistantPlugin
};
}) as ObservabilityAIAssistantRouteHandlerResources['plugins'];

// Using once to make sure the same model ID is used during service init and Knowledge base setup
const getModelId = once(async () => {
// Using once to make sure the same model ID is used during service init and Knowledge base setup
const defaultModelId = '.elser_model_2';
const [_, pluginsStart] = await core.getStartServices();
const license = await firstValueFrom(pluginsStart.licensing.license$);

if (!license.hasAtLeast('enterprise')) {
Copy link
Member

Choose a reason for hiding this comment

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

Platinum?

Copy link
Member Author

@sorenlouv sorenlouv Apr 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I was not sure. We require enterprise everywhere else. I haven't seen a reference to platinum:

kibana/x-pack/plugins/observability_solution/observability_ai_assistant/server/plugin.ts

Line 69 in 12ecb70

minimumLicense: 'enterprise',

Copy link
Member

Choose a reason for hiding this comment

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

yes, but I assume this throws because users don't have a platinum license (for ML). maybe we should guard elsewhere 🤔 maybe in /server/service/index.ts:doInit we should catch the error, WDYT?

Copy link
Member Author

@sorenlouv sorenlouv Apr 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, the error is thrown because of a check in ML but regardless we should never attempt to do this in the first place if the customer does not have at least enterprise license (which is the required license for AI Assistant).

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe we should guard elsewhere 🤔 maybe in /server/service/index.ts:doInit we should catch the error, WDYT?

What about setup then?

kibana/x-pack/plugins/observability_solution/observability_ai_assistant/server/service/knowledge_base_service/index.ts

Lines 80 to 81 in cff6de2

setup = async () => {
const elserModelId = await this.dependencies.getModelId();

or status?

kibana/x-pack/plugins/observability_solution/observability_ai_assistant/server/service/knowledge_base_service/index.ts

Lines 273 to 274 in cff6de2

status = async () => {
const elserModelId = await this.dependencies.getModelId();

Copy link
Member

Choose a reason for hiding this comment

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

yeah this is a good point, will probably get hairy. I think it's fine as is then!

return defaultModelId;
}

try {
// Wait for the ML plugin's dependency on the internal saved objects client to be ready
const [_, pluginsStart] = await core.getStartServices();

const { ml } = await core.plugins.onSetup('ml');

if (!ml.found) {
throw new Error('Could not find ML plugin');
}

// Wait for the license to be available so the ML plugin's guards pass once we ask for ELSER stats
await firstValueFrom(pluginsStart.licensing.license$);

const elserModelDefinition = await (
ml.contract as {
trainedModelsProvider: (
Expand All @@ -137,9 +139,7 @@ export class ObservabilityAIAssistantPlugin
return elserModelDefinition.model_id;
} catch (error) {
this.logger.error(`Failed to resolve ELSER model definition: ${error}`);

// Fallback to ELSER v2
return '.elser_model_2';
return defaultModelId;
}
});

Expand Down