1618115 - tips/interventions -shown telemetry should not be incremented in the provider

Status: NEW

(Firefox :: Address Bar, defect, P3)

Description

[Marco Bonardo [:mak]]

The fact the provider returns a tip/intervention is not enough to tell whether it was shown, it's possible in the meanwhile the search was canceled and the providersManager will ignore those results.
We should probably increment telemetry in the frontend when tips/interventions are effectively shown.

Comment 1

[Drew Willcoxon :adw]

See also bug 1617318. Interventions will be tricky because we want to increment the scalar only once per engagement.

Comment 2

[Marco Bonardo [:mak]]

Can we use my suggestion of returning a bool from addCallback, that states whether the providersManager forwarded the result, in that case?

Comment 3

[Drew Willcoxon :adw]

Wouldn't that be tricky for providers that aren't immediate? Because then it depends on when/whether the notifyResults timer is called, so it will at least be async.

My idea would be to have the view notify the provider (by calling a method on it) that its results were shown. It would do it in _updateRow. The interventions provider would batch these notifications per engagement.

Thinking along those lines, we might still be able to do bug 1617318. Yes, the interventions provider needs to batch its shown-counts per engagement -- but the search tips provider could do that too, it doesn't make a difference.

Comment 4

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #3)

Wouldn't that be tricky for providers that aren't immediate? Because then it depends on when/whether the notifyResults timer is called, so it will at least be async.

yes, it wouldn't be 100% perfect, but it would still be better than incrementing regardless before invoking add, that may bailout immediately.

My idea would be to have the view notify the provider (by calling a method on it) that its results were shown. It would do it in _updateRow. The interventions provider would batch these notifications per engagement.

It would surely be more correct, but it's also quite a lot of complication to avoid edge cases, and may have a performance cost if the view must notify each provider. In general I still think that we should limit cycles between the view and the model, as far as possible, that allows for a cleaner abstraction layer between them.
I may be overzealous, if you think it's not too much of a problem, feel free to propose it as a change.