Bug 648121 (telemetrya11y)

Accessibility Telemetry

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: davidb, Assigned: tbsaunde)

Tracking

({meta})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

We are soon going to get data on whether our test pilots have accessibility instantiated. We should also collect anonymous information on what might have caused the instantiation (e.g. detection based on process-resident modules).
Morphing this bug since I think we want to use Telemetry!
https://wiki.mozilla.org/Platform/Features/Telemetry

Specifically, we should be able to collect data on what parts of our API is being used, and what isn't. Cross referencing this by tool/technology would be invaluable.
Assignee: nobody → bolterbugz
Summary: Provide accessibility API for test pilot → Accessibility Telemetry
Taras could you comment on whether telemetry is the right tool for gathering this kind of data:

- whether accessibility is instantiated (since it is lazily instantiated)
- what part of our API is being used (via instrumentation)
- information about known injected modules (i.e. identify known screen readers and other causes of a11y invocation)

If so, in terms of scheduling would we be able to make this a Q3 a11y goal?

Comment 3

8 years ago
This all sounds reasonable for telemetry. Metrics that poll hardware may or may not need privacy/security review. We haven't figured that out yet. Overall this this sounds doable for q3.
Nice progress! Can you point me to a try build? How do I test this?
Assignee

Comment 6

8 years ago
(In reply to comment #5)
> Nice progress! Can you point me to a try build? How do I test this?

well, I can make one, but don't have one on hand.  I think you also need to grab a addon about:Telemetry  iirc Taras?
I'm not sure what is Telemetry API and what ways it provides to display information. But it sounds for me that gathered information may be interesting rather than useful because all it can provide is amount of created/destroyed accessible on time axis. No memory numbers, no information per tab/document.

Btw, is telemetry part of the build? Is there a way to not accumulate information if nobody needs it?
(In reply to comment #7)
> Btw, is telemetry part of the build? Is there a way to not accumulate
> information if nobody needs it?

There is still discussion about the UI for how the user controls telemetry, but right now there is a setting in Preferences->Advanced called "submit performance data". I think if this is turned off then the information does not accumulate.
(In reply to comment #8)
> (In reply to comment #7)
> > Btw, is telemetry part of the build? Is there a way to not accumulate
> > information if nobody needs it?
> 
> There is still discussion about the UI for how the user controls telemetry,
> but right now there is a setting in Preferences->Advanced called "submit
> performance data". I think if this is turned off then the information does
> not accumulate.

maybe it's not stored but processing is running all time, I concerned to last one.
(In reply to comment #9)

> maybe it's not stored but processing is running all time, I concerned to
> last one.

You end up with a cost of a couple of function calls per probe when telemetry is deactivated. I think we can afford that :)
This depends on what kind of info we accumulate, maybe not big anyway. But I think I would feel better if we put this code under 'if' to avoid extra calls for things that are never used in most of cases (assuming it's turned off by default for most of users). Btw, is telemetry always part of release versions of Mozilla products (like Firefox, Thuderbird, something else) so we don't need to ifdef this code?
(In reply to comment #11)
> This depends on what kind of info we accumulate, maybe not big anyway. But I
> think I would feel better if we put this code under 'if' to avoid extra
> calls for things that are never used in most of cases (assuming it's turned
> off by default for most of users). Btw, is telemetry always part of release
> versions of Mozilla products (like Firefox, Thuderbird, something else) so
> we don't need to ifdef this code?

You do no need to if/ifdef it. If a couple of function calls are weighing you down, then you are putting telemetry code in the wrong place.
Assignee

Comment 13

8 years ago
(In reply to comment #12)
> (In reply to comment #11)
> > This depends on what kind of info we accumulate, maybe not big anyway. But I
> > think I would feel better if we put this code under 'if' to avoid extra
> > calls for things that are never used in most of cases (assuming it's turned
> > off by default for most of users). Btw, is telemetry always part of release
> > versions of Mozilla products (like Firefox, Thuderbird, something else) so
> > we don't need to ifdef this code?
> 
> You do no need to if/ifdef it. If a couple of function calls are weighing
> you down, then you are putting telemetry code in the wrong place.

So I think we'd like to be able to measure rates, for example number of accessibles created per  second.  That would seem to mean we want to have a function  getting called by a nsITimer every so often.  The thing here is it seems suboptimal to have a timer wake us up if we are running on a laptop or mobile device with firefox in a suspended / backgrounded state, it seems like that would be unneeded battery use, and we should only set the timer up if telemetry is running.  it may be that in some cases time based isn't really the most sensible thing, but this seems like a fairly reasonable thing to want to do.
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > This depends on what kind of info we accumulate, maybe not big anyway. But I
> > > think I would feel better if we put this code under 'if' to avoid extra
> > > calls for things that are never used in most of cases (assuming it's turned
> > > off by default for most of users). Btw, is telemetry always part of release
> > > versions of Mozilla products (like Firefox, Thuderbird, something else) so
> > > we don't need to ifdef this code?
> > 
> > You do no need to if/ifdef it. If a couple of function calls are weighing
> > you down, then you are putting telemetry code in the wrong place.
> 
> So I think we'd like to be able to measure rates, for example number of
> accessibles created per  second.  That would seem to mean we want to have a
> function  getting called by a nsITimer every so often.  The thing here is it
> seems suboptimal to have a timer wake us up if we are running on a laptop or
> mobile device with firefox in a suspended / backgrounded state, it seems
> like that would be unneeded battery use, and we should only set the timer up
> if telemetry is running.  it may be that in some cases time based isn't
> really the most sensible thing, but this seems like a fairly reasonable
> thing to want to do.

I don't see why you need timers for this. Just check a timestamp on accessible creation.
Assignee

Comment 16

8 years ago
Posted patch accessible life time v1 (obsolete) — Splinter Review
Attachment #550951 - Flags: review?(surkov.alexander)
Assignee

Comment 17

8 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #16)
> Created attachment 550951 [details] [diff] [review] [diff] [details] [review]
> accessible life time v1

i STARTED PUTTING TELEMETRY RELATED STUFF IN A MOZILLA::A11Y::STATISTICS NAMESPACE (WHICH aLEX MENTIONED AS AN IDEA).  i THINK  THIS MAY ALSO SERVE WELL FOR ADDING MEMORY REPORTS FOR ACCESSIBLES, PARTICULAR IF WE ADD  A SIZE ARG TO aCCESSIBLEaDDED() AND aCCESSIBLEdESTROYED() WE CAN TRACK THE TOTAL SIZE OF ALL THE ACCESSIBLES.
Assignee

Comment 18

8 years ago
(In reply to alexander surkov from comment #7)
> I'm not sure what is Telemetry API and what ways it provides to display
> information. But it sounds for me that gathered information may be
> interesting rather than useful because all it can provide is amount of
> created/destroyed accessible on time axis. No memory numbers, no information

Well, since  most accessibles are about the same size, nsAccessible is 128 bytes and the biggest subtype I could find was 160 on a x64 linux build we can get a pretty good gues from the number of accessibles.

also, it seems like it ight be pretty useful to understnad what the rates of creation and destruction are like, and how long accessibles are in use for.  For exaple we could aybe consider reusing an accessible if it turns out they're used for a very short period.

> per tab/document.

how useful is it to know how accessibles are appropriated among the documents?
I can see only alive accessible historgram, I don't understand what's on x and y axes and not sure how we can use the info: 192 samples, average = 2295.5, sum = 440741. Ok, we created 440741 accessible objects but what's next? Also ACCESSIBLE_LIFETIME histogram is rather useless - while we spend 4 bytes per each object - we get questionable info how long accessible objects live. For example, if tab isn't closed for weeks then they live for weeks; if I search then they are created and destroyed every dozen seconds when I open and close tabs, so we get an average I assume and then think why we we would need that.
(In reply to alexander surkov from comment #19)
> I can see only alive accessible historgram, I don't understand what's on x
> and y axes

is x amount of created accessible and is y time spent for that?
(In reply to alexander surkov from comment #20)
> (In reply to alexander surkov from comment #19)
> > I can see only alive accessible historgram, I don't understand what's on x
> > and y axes
> 
> is x amount of created accessible and is y time spent for that?

sounds like no since time is not accumulated for this histogram.
Assignee

Comment 22

8 years ago
(In reply to alexander surkov from comment #19)
> I can see only alive accessible historgram, I don't understand what's on x
> and y axes and not sure how we can use the info: 192 samples, average =
> 2295.5, sum = 440741. Ok, we created 440741 accessible objects but what's
> next? Also ACCESSIBLE_LIFETIME histogram is rather useless - while we spend

the number of accessibles seems atleast somewhat useful in informing decissions about what data fields we add or remove from accessibles.  if we have roughly 2300 accessibles with an accessible being about 140 bytes that's  about 330 kb for the accessibles ignoring the arrays of children hash table's mapping content to accessibles.    Now we also know that changing the size of nsAccessible by 4 bytes will be a total change of about 10kb.  Of course this is most useful if we have the number of tabs open too, I'm not sure if we currently collect that or not.

> 4 bytes per each object - we get questionable info how long accessible
> objects live. For example, if tab isn't closed for weeks then they live for
> weeks; if I search then they are created and destroyed every dozen seconds
> when I open and close tabs, so we get an average I assume and then think why
> we we would need that.

I'd agree the average doesn't seem useful here, but it seems the extent of the curve might be somewhat, especially wrt layout doing tricky things that cause us  to remove accessibles in places we wouldn't immediately expect that.
(In reply to Trevor Saunders (:tbsaunde) from comment #22)

> the number of accessibles seems atleast somewhat useful in informing
> decissions about what data fields we add or remove from accessibles.  if we
> have roughly 2300 accessibles with an accessible being about 140 bytes
> that's  about 330 kb for the accessibles ignoring the arrays of children
> hash table's mapping content to accessibles.    Now we also know that
> changing the size of nsAccessible by 4 bytes will be a total change of about
> 10kb.  Of course this is most useful if we have the number of tabs open too,
> I'm not sure if we currently collect that or not.

one million of open and closed tabs containing one accessible results to one million created and destroyed accessible and make your chart report about one million accessible and it doesn't give us good information about memory usage. Having said that alive accessible (not created) provides more sensible information. But also you should know amount of accessible of each type to do assumption about fields. Maybe about:memory suites better for this tasks.

> > 4 bytes per each object - we get questionable info how long accessible
> > objects live. For example, if tab isn't closed for weeks then they live for
> > weeks; if I search then they are created and destroyed every dozen seconds
> > when I open and close tabs, so we get an average I assume and then think why
> > we we would need that.
> 
> I'd agree the average doesn't seem useful here, but it seems the extent of
> the curve might be somewhat, especially wrt layout doing tricky things that
> cause us  to remove accessibles in places we wouldn't immediately expect
> that.

how can curve tell you there's something tricky with layout rather than user just closed the tab for example?

In general I need valid scenario how the specific measure can help us. For example, I suggested to measure timing rather than accessibles count (like tree creation, event coalescence, text difference) so that if we can see it can take long time then we have a problem. We could analyze the code trying to improve numbers or gather URIs where our code takes long time (not sure it suites well the Telemetry though).

Also it makes sense to count calls count of deprecated methods (like methods IAccessibleTable interface) and not implemented methods (like get_accHelp or accNavigate with NAVDIR_LEFT and etc).
Assignee

Comment 24

8 years ago
Posted patch a11y initialization (obsolete) — Splinter Review
Attachment #546562 - Attachment is obsolete: true
Attachment #550951 - Attachment is obsolete: true
Attachment #550951 - Flags: review?(surkov.alexander)
Attachment #552381 - Flags: review?(surkov.alexander)
Comment on attachment 552381 [details] [diff] [review]
a11y initialization

>+HISTOGRAM(A11Y_INITIALIZATION, 0, 1, 2, BOOLEAN, "if accessibility suport has been initialized")

Drive by nit: maybe better to say "Has accessibility support been instantiated".
Assignee: bolterbugz → trev.saunders
Assignee

Updated

8 years ago
Attachment #552381 - Flags: review?(bolterbugz)
Assignee

Comment 26

8 years ago
(In reply to David Bolter [:davidb] from comment #25)
> Comment on attachment 552381 [details] [diff] [review]
> a11y initialization
> 
> >+HISTOGRAM(A11Y_INITIALIZATION, 0, 1, 2, BOOLEAN, "if accessibility suport has been initialized")
> 
> Drive by nit: maybe better to say "Has accessibility support been
> instantiated".

I considered that, and am willing to make that change, but technically we're looking at when we tried to initialize accessibility,  and if it worked or not.  Just we really only care about how many times it worked, and don't really expect it to ever fail.
Can we morph this bug to meta (and file dependences) or rename it properly if this patch is all we want to add for the first round? Trevor, your call?
Comment on attachment 552381 [details] [diff] [review]
a11y initialization

Review of attachment 552381 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for a11y part, let's get Taras review for Telemetry part to make sure you do right things

::: accessible/src/base/Statistics.h
@@ +61,5 @@
>    inline void AccessibleDeleted(PRUint32 aSize)
>      { gAccessiblesMemory -= aSize; }
>  
> +  inline void A11yInitialized(bool aWasSuccessful)
> +    { Telemetry::Accumulate(Telemetry::A11Y_INITIALIZATION, aWasSuccessful); }

A11Y_INSTANTIATED maybe?

we don't need aWasSuccessful, write down when a11y was instantiated successfully only

::: accessible/src/base/nsAccessibilityService.cpp
@@ +1768,5 @@
>    nsRefPtr<nsAccessibilityService> service = new nsAccessibilityService();
>    NS_ENSURE_TRUE(service, NS_ERROR_OUT_OF_MEMORY);
>  
>    if (!service->Init()) {
> +    statistics::A11yInitialized(false);

so remove this one
Attachment #552381 - Flags: review?(tglek)
Attachment #552381 - Flags: review?(surkov.alexander)
Attachment #552381 - Flags: review?(bolterbugz)
Attachment #552381 - Flags: review+

Updated

8 years ago
Attachment #552381 - Flags: review?(tglek) → review+
Assignee

Comment 29

8 years ago
Posted patch patch v.1Splinter Review
comments should be addressed, and moved a round a little goo, but basically the same
Attachment #552381 - Attachment is obsolete: true
Attachment #552833 - Flags: review?(surkov.alexander)
Assignee

Comment 30

8 years ago
(In reply to alexander surkov from comment #27)
> Can we morph this bug to meta (and file dependences) or rename it properly
> if this patch is all we want to add for the first round? Trevor, your call?

I think a meta bug might make sense to keep them together so metaifying
Keywords: meta
OS: Windows 7 → All
Comment on attachment 552833 [details] [diff] [review]
patch v.1

Review of attachment 552833 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/Statistics.h
@@ +42,5 @@
> +
> +#include "mozilla/Telemetry.h"
> +
> +namespace mozilla {
> +namespace a11y {

do we really should put it under mozilla::a11y since it's not exported?

@@ +45,5 @@
> +namespace mozilla {
> +namespace a11y {
> +namespace statistics {
> +
> +  inline void A11yInitialized()

maybe A11yInstantiated like telemetry constant?

@@ +51,5 @@
> +
> +}
> +}
> +}
> +

nit: comment which namespece each brace belongs to

::: accessible/src/base/nsAccessibilityService.cpp
@@ +1772,5 @@
>      service->Shutdown();
>      return NS_ERROR_FAILURE;
>    }
>  
> +  statistics::A11yInitialized();

nit: new line?
Attachment #552833 - Flags: review?(surkov.alexander) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #30)
> (In reply to alexander surkov from comment #27)
> > Can we morph this bug to meta (and file dependences) or rename it properly
> > if this patch is all we want to add for the first round? Trevor, your call?
> 
> I think a meta bug might make sense to keep them together so metaifying

then it makes sense to file new bug to give new home for your patch and keep this one for generic discussions only or morph this bug for this patch and file new meta bug
Assignee

Comment 33

8 years ago
(In reply to alexander surkov from comment #32)
> (In reply to Trevor Saunders (:tbsaunde) from comment #30)
> > (In reply to alexander surkov from comment #27)
> > > Can we morph this bug to meta (and file dependences) or rename it properly
> > > if this patch is all we want to add for the first round? Trevor, your call?
> > 
> > I think a meta bug might make sense to keep them together so metaifying
> 
> then it makes sense to file new bug to give new home for your patch and keep
> this one for generic discussions only or morph this bug for this patch and
> file new meta bug

well, filing a new bug to which I will attach a patch and then immediately  resolve fixed seems a little silly...  On the other hand I think there is some useful generic discussion in this bug, so maybe morphing this bug and creating a new meta bug isn't a great idea either.

> @@ +42,5 @@
> > +
> > +#include "mozilla/Telemetry.h"
> > +
> > +namespace mozilla {
> > +namespace a11y {
> 
> do we really should put it under mozilla::a11y since it's not exported?

well, since the only thing in the namespace atm is a inline function probably not since no symbols will be produced, but if we start adding things that will create symbols then it probably should be in the mozilla::a11y namespace so that if say there is a statistics namespace in content/ somewhere its symbols will be seperate from ours.

> @@ +45,5 @@
> > +namespace mozilla {
> > +namespace a11y {
> > +namespace statistics {
> > +
> > +  inline void A11yInitialized()
> 
> maybe A11yInstantiated like telemetry constant?

sure

> @@ +51,5 @@
> > +
> > +}
> > +}
> > +}
> > +
> 
> nit: comment which namespece each brace belongs to

ok, sorry I always forget that one, my editor has jump to opposing brace ;)

> ::: accessible/src/base/nsAccessibilityService.cpp
> @@ +1772,5 @@
> >      service->Shutdown();
> >      return NS_ERROR_FAILURE;
> >    }
> >  
> > +  statistics::A11yInitialized();
> 
> nit: new line?

seems unneeded to me, but sure
(In reply to Trevor Saunders (:tbsaunde) from comment #33
> well, filing a new bug to which I will attach a patch and then immediately 
> resolve fixed seems a little silly...  On the other hand I think there is
> some useful generic discussion in this bug, so maybe morphing this bug and
> creating a new meta bug isn't a great idea either.

ok, open following ups when this patch is finshed/landed.
Assignee

Updated

8 years ago
Depends on: 678965
Assignee

Updated

8 years ago
Depends on: 678799
Assignee

Updated

8 years ago
Depends on: 679786
Alias: telemetrya11y
Depends on: 682247
Candidate: XForms. Can we remove support?
Assignee

Updated

8 years ago
Depends on: 717506
Depends on: 728904
Depends on: 728905
Depends on: 730198
Depends on: 732310
Assignee

Updated

7 years ago
Depends on: 776081
There are five Telemetry histograms covering aspects of a11y: A11Y_INSTANTIATED_FLAG, A11Y_CONSUMERS, A11Y_ISIMPLEDOM_USAGE_FLAG, A11Y_IATABLE_USAGE_FLAG, A11Y_UPDATE_TIME

Do they cover the needs of this bug?
Flags: needinfo?(dbolter)
(In reply to Chris H-C :chutten from comment #36)
> There are five Telemetry histograms covering aspects of a11y:
> A11Y_INSTANTIATED_FLAG, A11Y_CONSUMERS, A11Y_ISIMPLEDOM_USAGE_FLAG,
> A11Y_IATABLE_USAGE_FLAG, A11Y_UPDATE_TIME
> 
> Do they cover the needs of this bug?

Yeah I think this meta bug has served its purpose.
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(dbolter)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.