● PHANTOM
🇮🇳 IN
✕
Skip to content

Add credential manager to AIChatMetrics #34201

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
DJAndries wants to merge 1 commit into
base: master
Choose a base branch
from
+155 −104
Open

Add credential manager to AIChatMetrics #34201

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
18 changes: 11 additions & 7 deletions browser/ai_chat/ai_chat_metrics_browsertest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,19 @@

#include <memory>

#include "base/test/bind.h"
#include "base/test/metrics/histogram_tester.h"
#include "base/time/time.h"
#include "brave/app/brave_command_ids.h"
#include "brave/browser/misc_metrics/profile_misc_metrics_service.h"
#include "brave/browser/misc_metrics/profile_misc_metrics_service_factory.h"
#include "brave/browser/ui/brave_browser.h"
#include "brave/components/ai_chat/core/common/mojom/ai_chat.mojom.h"
#include "brave/components/ai_chat/core/common/mojom/common.mojom.h"
#include "brave/components/ai_chat/core/common/pref_names.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h"
#include "chrome/test/base/in_process_browser_test.h"
#include "components/prefs/pref_service.h"
#include "content/public/test/browser_test.h"

namespace ai_chat {
Expand All @@ -29,6 +32,12 @@ class AIChatMetricsTest : public InProcessBrowserTest {

void SetUpOnMainThread() override {
InProcessBrowserTest::SetUpOnMainThread();
// Simulate opted-in and a recent premium check so RecordEnabled proceeds
// without blocking on async credential or disclaimer checks.
browser()->profile()->GetPrefs()->SetTime(prefs::kLastAcceptedDisclaimer,
base::Time::Now());
g_browser_process->local_state()->SetTime(
prefs::kBraveChatP3ALastPremiumCheck, base::Time::Now());
content::ContextMenuParams params;
params.is_editable = false;
params.selection_text = u"some text";
Expand Down Expand Up @@ -60,12 +69,7 @@ IN_PROC_BROWSER_TEST_F(AIChatMetricsTest, ContextMenuActions) {
histogram_tester_.ExpectTotalCount(kMostUsedContextMenuActionHistogramName,
0);

ai_chat_metrics_->RecordEnabled(
true, true,
base::BindLambdaForTesting(
[&](mojom::Service::GetPremiumStatusCallback callback) {
std::move(callback).Run(mojom::PremiumStatus::Active, nullptr);
}));
ai_chat_metrics_->RecordEnabled(/*is_new_user=*/true);
histogram_tester_.ExpectUniqueSample(kMostUsedContextMenuActionHistogramName,
0, 1);

Expand Down
14 changes: 12 additions & 2 deletions browser/misc_metrics/profile_misc_metrics_service.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
#include "content/public/browser/browser_context.h"

#if BUILDFLAG(ENABLE_AI_CHAT)
#include "brave/browser/skus/skus_service_factory.h"
#include "brave/components/ai_chat/core/browser/ai_chat_credential_manager.h"
#include "brave/components/ai_chat/core/browser/ai_chat_metrics.h"
#endif // BUILDFLAG(ENABLE_AI_CHAT)

Expand All @@ -58,8 +60,16 @@ ProfileMiscMetricsService::ProfileMiscMetricsService(
base::Unretained(this)));
#if BUILDFLAG(ENABLE_AI_CHAT)
if (local_state) {
ai_chat_metrics_ =
std::make_unique<ai_chat::AIChatMetrics>(local_state, profile_prefs_);
auto skus_service_getter = base::BindRepeating(
[](content::BrowserContext* context) {
return skus::SkusServiceFactory::GetForContext(context);
},
context);
auto credential_manager =
std::make_unique<ai_chat::AIChatCredentialManager>(
std::move(skus_service_getter), local_state);
ai_chat_metrics_ = std::make_unique<ai_chat::AIChatMetrics>(
local_state, profile_prefs_, std::move(credential_manager));
}
#endif // BUILDFLAG(ENABLE_AI_CHAT)
}
Expand Down
8 changes: 8 additions & 0 deletions browser/misc_metrics/profile_misc_metrics_service_factory.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,18 @@

#include "base/no_destructor.h"
#include "brave/browser/misc_metrics/profile_misc_metrics_service.h"
#include "brave/components/ai_chat/core/common/buildflags/buildflags.h"
#include "chrome/browser/autofill/personal_data_manager_factory.h"
#include "chrome/browser/bookmarks/bookmark_model_factory.h"
#include "chrome/browser/content_settings/host_content_settings_map_factory.h"
#include "chrome/browser/history/history_service_factory.h"
#include "chrome/browser/search_engines/template_url_service_factory.h"
#include "components/keyed_service/content/browser_context_dependency_manager.h"

#if BUILDFLAG(ENABLE_AI_CHAT)
#include "brave/browser/skus/skus_service_factory.h"
#endif // BUILDFLAG(ENABLE_AI_CHAT)

#if !BUILDFLAG(IS_ANDROID)
#include "chrome/browser/themes/theme_service_factory.h"
#include "extensions/browser/extension_registry_factory.h"
Expand Down Expand Up @@ -50,6 +55,9 @@ ProfileMiscMetricsServiceFactory::ProfileMiscMetricsServiceFactory()
#else
DependsOn(SearchEngineTrackerFactory::GetInstance());
#endif
#if BUILDFLAG(ENABLE_AI_CHAT)
DependsOn(skus::SkusServiceFactory::GetInstance());
#endif // BUILDFLAG(ENABLE_AI_CHAT)
DependsOn(HistoryServiceFactory::GetInstance());
DependsOn(HostContentSettingsMapFactory::GetInstance());
DependsOn(autofill::PersonalDataManagerFactory::GetInstance());
Expand Down
6 changes: 4 additions & 2 deletions browser/misc_metrics/sources.gni
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,10 @@ brave_browser_misc_metrics_deps = [
]

if (enable_ai_chat) {
brave_browser_misc_metrics_deps +=
[ "//brave/components/ai_chat/core/browser" ]
brave_browser_misc_metrics_deps += [
"//brave/browser/skus",
"//brave/components/ai_chat/core/browser",
]
}

if (!is_android) {
Expand Down
58 changes: 34 additions & 24 deletions components/ai_chat/core/browser/ai_chat_metrics.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "base/metrics/histogram_functions_internal_overloads.h"
#include "base/metrics/histogram_macros.h"
#include "base/time/time.h"
#include "brave/components/ai_chat/core/browser/utils.h"
#include "brave/components/ai_chat/core/common/mojom/ai_chat.mojom-shared.h"
#include "brave/components/ai_chat/core/common/pref_names.h"
#include "brave/components/p3a_utils/bucket.h"
Expand Down Expand Up @@ -182,8 +183,10 @@ constexpr auto kUsageDailyHistogramNames =

} // namespace

AIChatMetrics::AIChatMetrics(PrefService* local_state,
PrefService* profile_prefs)
AIChatMetrics::AIChatMetrics(
PrefService* local_state,
PrefService* profile_prefs,
std::unique_ptr<AIChatCredentialManager> credential_manager)
: is_premium_(
local_state->GetBoolean(prefs::kBraveChatP3ALastPremiumStatus)),
chat_count_storage_(local_state,
Expand All @@ -210,7 +213,9 @@ AIChatMetrics::AIChatMetrics(PrefService* local_state,
prefs::kBraveChatP3AFullPageSwitches),
#endif // !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_IOS)
local_state_(local_state),
profile_prefs_(profile_prefs) {
profile_prefs_(profile_prefs),
credential_manager_(std::move(credential_manager)) {
CHECK(credential_manager_);
#if !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_IOS)
for (int i = 0; i <= static_cast<int>(ContextMenuAction::kMaxValue); i++) {
ContextMenuAction action = static_cast<ContextMenuAction>(i);
Expand Down Expand Up @@ -273,10 +278,25 @@ void AIChatMetrics::Bind(mojo::PendingReceiver<mojom::Metrics> receiver) {
receivers_.Add(this, std::move(receiver));
}

void AIChatMetrics::RecordEnabled(
bool is_enabled,
bool is_new_user,
RetrievePremiumStatusCallback retrieve_premium_status_callback) {
bool AIChatMetrics::MaybeUpdatePremiumStatus(std::optional<bool> is_new_user) {
base::Time last_premium_check =
local_state_->GetTime(prefs::kBraveChatP3ALastPremiumCheck);
if (!last_premium_check.is_null() &&
(base::Time::Now() - last_premium_check) < kPremiumCheckInterval) {
return false;
}
if (!premium_check_in_progress_) {
premium_check_in_progress_ = true;
credential_manager_->GetPremiumStatus(
base::BindOnce(&AIChatMetrics::OnPremiumStatusUpdated,
weak_ptr_factory_.GetWeakPtr(), is_new_user));
}
return true;
}

void AIChatMetrics::RecordEnabled(bool is_new_user) {
bool is_enabled = HasUserOptedIn(profile_prefs_);

if (is_enabled && !is_new_user &&
local_state_->GetTime(prefs::kBraveChatP3AFirstUsageTime).is_null()) {
// If the user already had AI chat enabled, and we did not record the first
Expand All @@ -292,20 +312,8 @@ void AIChatMetrics::RecordEnabled(
return;
}

if (retrieve_premium_status_callback) {
base::Time last_premium_check =
local_state_->GetTime(prefs::kBraveChatP3ALastPremiumCheck);
if (last_premium_check.is_null() ||
(base::Time::Now() - last_premium_check) >= kPremiumCheckInterval) {
if (!premium_check_in_progress_) {
premium_check_in_progress_ = true;
std::move(retrieve_premium_status_callback)
.Run(base::BindOnce(&AIChatMetrics::OnPremiumStatusUpdated,
weak_ptr_factory_.GetWeakPtr(), is_enabled,
is_new_user));
}
return;
}
if (MaybeUpdatePremiumStatus(is_new_user)) {
return;
}

is_enabled_ = true;
Expand All @@ -327,8 +335,7 @@ void AIChatMetrics::RecordReset() {
std::numeric_limits<int>::max() - 1, 7);
}

void AIChatMetrics::OnPremiumStatusUpdated(bool is_enabled,
bool is_new_user,
void AIChatMetrics::OnPremiumStatusUpdated(std::optional<bool> is_new_user,
mojom::PremiumStatus premium_status,
mojom::PremiumInfoPtr) {
is_premium_ = premium_status == mojom::PremiumStatus::Active ||
Expand All @@ -337,7 +344,9 @@ void AIChatMetrics::OnPremiumStatusUpdated(bool is_enabled,
local_state_->SetTime(prefs::kBraveChatP3ALastPremiumCheck,
base::Time::Now());
premium_check_in_progress_ = false;
RecordEnabled(is_enabled, is_new_user, {});
if (is_new_user.has_value()) {
RecordEnabled(*is_new_user);
}
}

void AIChatMetrics::RecordNewPrompt(ConversationHandlerForMetrics* handler,
Expand Down Expand Up @@ -520,6 +529,7 @@ void AIChatMetrics::ReportAllMetrics() {
periodic_report_timer_.Start(
FROM_HERE, base::Time::Now() + kReportInterval,
base::BindOnce(&AIChatMetrics::ReportAllMetrics, base::Unretained(this)));
MaybeUpdatePremiumStatus(std::nullopt);
ReportChatCounts();
ReportFeatureUsageMetrics();
ReportContextSource();
Expand Down
27 changes: 14 additions & 13 deletions components/ai_chat/core/browser/ai_chat_metrics.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,11 @@
#include <string>

#include "base/containers/flat_map.h"
#include "base/functional/callback.h"
#include "base/memory/raw_ptr.h"
#include "base/memory/weak_ptr.h"
#include "base/timer/timer.h"
#include "base/timer/wall_clock_timer.h"
#include "brave/components/ai_chat/core/browser/ai_chat_credential_manager.h"
Copy link
Collaborator

@netzenbot netzenbot Feb 26, 2026

Choose a reason for hiding this comment

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

AIChatCredentialManager is only used via std::unique_ptr in this header, and ~AIChatMetrics() is defined in the .cc file. A forward declaration would suffice here and avoid pulling in heavy transitive includes from ai_chat_credential_manager.h (skus mojom, mojo remote/remote_set headers). Move the full #include to the .cc file. best practice

Copy link
Collaborator Author

@DJAndries DJAndries Feb 26, 2026

Choose a reason for hiding this comment

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

If we were including headers from another non-base target, I would agree, but this sort of include is quite ubiquitous.

#include "brave/components/ai_chat/core/browser/ai_chat_tab_focus_metrics.h"
#include "brave/components/ai_chat/core/browser/skills_metrics.h"
#include "brave/components/ai_chat/core/common/mojom/ai_chat.mojom.h"
Expand Down Expand Up @@ -135,10 +135,9 @@ class AIChatMetrics : public mojom::Metrics,
public AIChatTabFocusMetrics::Delegate,
public SkillsMetrics::Delegate {
public:
using RetrievePremiumStatusCallback =
base::OnceCallback<void(mojom::Service::GetPremiumStatusCallback)>;

AIChatMetrics(PrefService* local_state, PrefService* profile_prefs);
AIChatMetrics(PrefService* local_state,
PrefService* profile_prefs,
std::unique_ptr<AIChatCredentialManager> credential_manager);
~AIChatMetrics() override;

AIChatMetrics(const AIChatMetrics&) = delete;
Expand All @@ -152,10 +151,7 @@ class AIChatMetrics : public mojom::Metrics,
return tab_focus_metrics_.get();
}

void RecordEnabled(
bool is_enabled,
bool is_new_user,
RetrievePremiumStatusCallback retrieve_premium_status_callback);
void RecordEnabled(bool is_new_user);
void RecordReset();

void RecordNewPrompt(ConversationHandlerForMetrics* handler,
Expand All @@ -174,10 +170,6 @@ class AIChatMetrics : public mojom::Metrics,
void RecordFullPageSwitch();
#endif // !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_IOS)

void OnPremiumStatusUpdated(bool is_enabled,
bool is_new_user,
mojom::PremiumStatus premium_status,
mojom::PremiumInfoPtr);
void MaybeRecordLastError(ConversationHandlerForMetrics* handler);

// Metrics:
Expand Down Expand Up @@ -209,6 +201,13 @@ class AIChatMetrics : public mojom::Metrics,
void ReportFullPageUsageMetric();
#endif // !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_IOS)

// If is_new_user is provided, RecordEnabled will be called after the check.
bool MaybeUpdatePremiumStatus(std::optional<bool> is_new_user);

void OnPremiumStatusUpdated(std::optional<bool> is_new_user,
mojom::PremiumStatus premium_status,
mojom::PremiumInfoPtr);

void MaybeReportFirstChatPrompts(bool new_prompt_made);
void RecordContextSource(ConversationHandlerForMetrics* handler,
mojom::ConversationTurnPtr& entry);
Expand Down Expand Up @@ -252,6 +251,8 @@ class AIChatMetrics : public mojom::Metrics,
raw_ptr<PrefService> local_state_;
raw_ptr<PrefService> profile_prefs_;

std::unique_ptr<AIChatCredentialManager> credential_manager_;
Copy link
Collaborator

@netzenbot netzenbot Feb 26, 2026

Choose a reason for hiding this comment

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

nit: credential_manager_ is an owning unique_ptr placed after the non-owning raw_ptr<PrefService> members. Per convention, owning members should come before unowned raw_ptr members to make ownership semantics visually clear. Consider grouping it with the other unique_ptr members (tab_focus_metrics_, skills_metrics_) below. best practice

Copy link
Collaborator Author

@DJAndries DJAndries Feb 26, 2026

Choose a reason for hiding this comment

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

Curious how this guideline came about. Is this an upstream code style matter?

Copy link
Member

@bbondy bbondy Feb 26, 2026

Choose a reason for hiding this comment

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

It's from https://chromium.googlesource.com/chromium/src/+/lkgr/docs/dangling_ptr_guide.md
It recommends placing owning members (std::unique_ptr, scoped_refptr) before unowned members (raw_ptr)

The rationale is about destruction safety: C++ destroys members in reverse declaration order, so placing raw_ptr members last means they are destroyed first. Before the owning members they may reference are freed. This prevents dangling pointer issues.


mojo::ReceiverSet<mojom::Metrics> receivers_;

std::unique_ptr<AIChatTabFocusMetrics> tab_focus_metrics_;
Expand Down
Loading
Loading