1352961 - Add a measure for the delay before a new tab starts processing the first URL

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement)

Description

[Gabor Krizsanits (INACTIVE)]

There are a lot of code paths between the moment when the user initiates the first page load in a new tab and the moment where we actually start processing that URL.

With e10s-multi this can spin up a new content process and load a bunch of process/frame scripts before the URL would be processed, which can get a lot worse if the user even have some slow add-ons.

We do have SIMPLE_MEASURES_FIRSTLOADURI which does not measure exactly what we want here, but even worse with the pre-allocated process manager it probably won't be very useful any longer.

Comment 1

[Gabor Krizsanits (INACTIVE)]

Hi, :mconley told me that you might be the person I should talk to about this. Do you think we have something already we could use for this?

Comment 2

[Gabor Krizsanits (INACTIVE)]

I think I will just start a timer here:
http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/toolkit/components/remotebrowserutils/RemoteWebNavigation.js#73

and end it here:
http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/toolkit/content/browser-child.js#266

That should measure the overhead an addition content process startup can cause for new tabs.

Only problem that we don't have a timer exposed to JS right now that can be used for this, so I will have to come up with something.

The real solution would be to teach TelemetryStopwatch to handle this, but that will be a followup probably.

Comment 3

[Gabor Krizsanits (INACTIVE)]

Attachment: P1: exposing msSystemNow to have a clock that can be used across the process boundaries. v1

This is not great I know. But there is just no timer that is monotonic AND can be used across the process boundaries (the epoch is not guaranteed to be the same system wide). I'm afraid bug 1354167 will end up using this timer too or something similar.

Comment 4

[Gabor Krizsanits (INACTIVE)]

Attachment: P2: FX_TAB_REMOTE_NAVIGATION_DELAY telemetry. v1

Mike, I hope that the patch comment is descriptive enough... This is the probe I talked to you about earlier when you explained me how all this front-end code works.

I've added Chris as well with f? to make sure that I'm doing the telemetry bits right.

Comment 5

[Gabor Krizsanits (INACTIVE)]

Attachment: P2: FX_TAB_REMOTE_NAVIGATION_DELAY telemetry. v2

I forgot to add the test to the previous patch. Sorry for the bug mail spam.

Comment 6

[Chris H-C :chutten]

Comment on attachment 8857016 [details] [diff] [review]
P1: exposing msSystemNow to have a clock that can be used across the process boundaries. v1

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

When I was working on bug 1235908 TimeStamps were considered valid cross-process on the Tier1 platforms. The comments in TimeStamp.cpp are encouraging as well (https://dxr.mozilla.org/mozilla-central/source/mozglue/misc/TimeStamp.h#381)

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8857020 [details] [diff] [review]
P2: FX_TAB_REMOTE_NAVIGATION_DELAY telemetry. v2

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

Makes sense to me - just a few things below. Thanks gabor!

::: dom/ipc/tests/browser.ini
@@ +6,5 @@
>  [browser_domainPolicy.js]
>  [browser_memory_distribution_telemetry.js]
>  skip-if = !e10 # This is an e10s only probe.
> +[browser_remote_navigation_delay_telemetry.js]
> +skip-if = !e10 # This is an e10s only probe.

!e10s

::: dom/ipc/tests/browser_remote_navigation_delay_telemetry.js
@@ +13,5 @@
> +      </body>
> +    </html>`;
> +
> +add_task(function* test_memory_distribution() {
> +  waitForExplicitFinish();

When using add_task, we don't need to tell the harness to wait for an explicit finish. It'll finish once this generator function finally exits, and we can yield Promises to go back to the event loop while we wait for things.

@@ +15,5 @@
> +
> +add_task(function* test_memory_distribution() {
> +  waitForExplicitFinish();
> +
> +  if (SpecialPowers.getIntPref("dom.ipc.processCount", 1) < 2) {

This is a browser mochitest, which means you have access to Services, so you can use Services.getIntPref.

@@ +17,5 @@
> +  waitForExplicitFinish();
> +
> +  if (SpecialPowers.getIntPref("dom.ipc.processCount", 1) < 2) {
> +    ok(true, "Skip this test if e10s-multi is disabled.");
> +    finish();

No need to finish - just exit.

@@ +24,5 @@
> +
> +  yield SpecialPowers.pushPrefEnv({set: [["toolkit.telemetry.enabled", true]]});
> +  Services.telemetry.canRecordExtended = true;
> +
> +  // Note the #content suffix after the id. This is the only way this API let us fetch the

Nit "let" -> "lets"

@@ +30,5 @@
> +  let histogram = Services.telemetry.getKeyedHistogramById("FX_TAB_REMOTE_NAVIGATION_DELAY#content");
> +  histogram.clear();
> +
> +  // Open a remote page in a new tab to trigger the WebNavigation:LoadURI.
> +  let tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser, DUMMY_PAGE_DATA_URI);

Probably should assert that tab1.linkedBrowser.isRemoteBrowser while we're here.

You can point the tab at "http://example.com". The test harness has a special internal page for that, and then you can get rid of your data URI.

@@ +33,5 @@
> +  // Open a remote page in a new tab to trigger the WebNavigation:LoadURI.
> +  let tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser, DUMMY_PAGE_DATA_URI);
> +
> +  // Open a new tab with about:robots, so it ends up in the parent process with a non-remote browser.
> +  let tab2 = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "about:robots");

Should assert that tab2.linkedBrowser.isRemoteBrowser is false.

@@ +35,5 @@
> +
> +  // Open a new tab with about:robots, so it ends up in the parent process with a non-remote browser.
> +  let tab2 = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "about:robots");
> +  // Navigate the tab, so it will change remotness and it triggers the SessionStore:restoreTabContent case.
> +  yield BrowserTestUtils.loadURI(tab2.linkedBrowser, DUMMY_PAGE_DATA_URI);

Use example.com here instead, I think.

@@ +39,5 @@
> +  yield BrowserTestUtils.loadURI(tab2.linkedBrowser, DUMMY_PAGE_DATA_URI);
> +
> +  // There is no good way to make sure that the parent received the histogram entries from the child processes.
> +  // Let's stick to the ugly, spinning the event loop until we're good approach.
> +  yield BrowserTestUtils.waitForCondition(() => {

You should wait for the <xul:tab> to fire the SSTabRestored event - this means that restoreTabContent has been received by content-sessionStore.js, and responded to - so at that point, the content process should know about the histogram entry.

Then we need to somehow force the content process to send its Telemetry up to the parent. I _think_ you could use this: http://searchfox.org/mozilla-central/rev/624d25b2980cf5f83452b040e6d664460f9b7ec5/toolkit/components/telemetry/nsITelemetry.idl#494-498 but you should double-check with chutten.

@@ +47,5 @@
> +
> +  let s = histogram.snapshot();
> +  let restoreTabSnapshot = s["SessionStore:restoreTabContent"];
> +  ok(restoreTabSnapshot.sum > 0, "Zero delay for the restoreTabContent case is unlikely.");
> +  ok(restoreTabSnapshot.sum < 10000, "More than 10 seconds delat for the restoreTabContent case is unlikely.");

typo: "delat" -> "delay"

@@ +51,5 @@
> +  ok(restoreTabSnapshot.sum < 10000, "More than 10 seconds delat for the restoreTabContent case is unlikely.");
> +
> +  let loadURISnapshot = s["WebNavigation:LoadURI"];
> +  ok(loadURISnapshot.sum > 0, "Zero delay for the LoadURI case is unlikely.");
> +  ok(loadURISnapshot.sum < 10000, "More than 10 seconds delat for the LoadURI case is unlikely.");

typo: "delat" -> "delay"

@@ +57,5 @@
> +  histogram.clear();
> +
> +  yield BrowserTestUtils.removeTab(tab2);
> +  yield BrowserTestUtils.removeTab(tab1);
> +  finish();

No need for the finish();

::: toolkit/components/telemetry/Histograms.json
@@ +5273,5 @@
>      "high": 500,
>      "n_buckets": 50,
>      "description": "Average frame interval during any tab open/close animation (excluding tabstrip scroll)"
>    },
> +  "FX_TAB_REMOTE_NAVIGATION_DELAY": {

You're going to need a data-review.

Comment 8

[Gabor Krizsanits (INACTIVE)]

(In reply to Chris H-C :chutten from comment #6)
> Comment on attachment 8857016 [details] [diff] [review]
> P1: exposing msSystemNow to have a clock that can be used across the process
> boundaries. v1
> 
> Review of attachment 8857016 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> When I was working on bug 1235908 TimeStamps were considered valid
> cross-process on the Tier1 platforms. The comments in TimeStamp.cpp are
> encouraging as well
> (https://dxr.mozilla.org/mozilla-central/source/mozglue/misc/TimeStamp.h#381)

The spec is less promising: https://linux.die.net/man/3/clock_gettime (we're using CLOCK_MONOTONIC for which the spec is vague enough not to specify if the |some unspecified starting point| is system wide or not). I'm not sure what is the right way to test this across different CPUs/Compilers/OS versions in various scenarios, but it's not like the system wide alternative is any better... As much as I can tell we are not the only ones who assumes that the epoch is the same for each process _always_ so I'd be OK with that.

The real problem is that TimeStamp is not exposed to JS, and I cannot just add a method that returns TimeStamp::Now() as double since that cannot be transformed to a time unit, only the difference of two TimeStamps can be converted to ms's.

So I would have to write a JS wrapper for the TimeStamp object and then make sure that sendAsyncMessage can serialize it. That seems like more trouble than it's worth given that we want to revamp this in bug 1354167. I would prefer actually to work on that bug first, but this probe is just too urgent for that. I feel like I'm missing something obvious here or just overestimate the time it takes to write that serializable wrapper. Anyway, let me know if we're on the same page.

Comment 9

[Gabor Krizsanits (INACTIVE)]

(In reply to Mike Conley (:mconley) from comment #7)
> @@ +39,5 @@
> > +  yield BrowserTestUtils.loadURI(tab2.linkedBrowser, DUMMY_PAGE_DATA_URI);
> > +
> > +  // There is no good way to make sure that the parent received the histogram entries from the child processes.
> > +  // Let's stick to the ugly, spinning the event loop until we're good approach.
> > +  yield BrowserTestUtils.waitForCondition(() => {
> 
> You should wait for the <xul:tab> to fire the SSTabRestored event - this
> means that restoreTabContent has been received by content-sessionStore.js,
> and responded to - so at that point, the content process should know about
> the histogram entry.
> 
> Then we need to somehow force the content process to send its Telemetry up
> to the parent. I _think_ you could use this:
> http://searchfox.org/mozilla-central/rev/
> 624d25b2980cf5f83452b040e6d664460f9b7ec5/toolkit/components/telemetry/
> nsITelemetry.idl#494-498 but you should double-check with chutten.


It's not so much about forcing the child to send the message with the telemetry data than about detecting when the parent receives that message. I could invent an observer notification for that... But I would prefer a more generic solution for this that goes beyond the scope of this bug. Either on top of Telemetry, or BrowserTestUtils to wait for a specific Histogram or other entry, filtered by the process type it was recorded. How about a followup?

> You're going to need a data-review.

Thanks for the reminder, I forgot about that for my previous probe, so it won't happen again :)

Comment 10

[Chris H-C :chutten]

Right, right. Being able to expose it to and translate it for JS was one of the requirements. I'm sorry, I forgot. Let me take another look.

Comment 11

[Chris H-C :chutten]

Comment on attachment 8857016 [details] [diff] [review]
P1: exposing msSystemNow to have a clock that can be used across the process boundaries. v1

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

If we have a bug for making TelemetryStopwatch work across processes, please include "remove msSystemNow" in the list of requirements.

If we don't have such a bug, please file one? :)

Comment 12

[Chris H-C :chutten]

Comment on attachment 8857020 [details] [diff] [review]
P2: FX_TAB_REMOTE_NAVIGATION_DELAY telemetry. v2

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

::: toolkit/components/telemetry/Histograms.json
@@ +5273,5 @@
>      "high": 500,
>      "n_buckets": 50,
>      "description": "Average frame interval during any tab open/close animation (excluding tabstrip scroll)"
>    },
> +  "FX_TAB_REMOTE_NAVIGATION_DELAY": {

Putting units (in this case "_MS") on the end is typical for probe names.

@@ +5278,5 @@
> +    "alert_emails": ["[e-mail]"],
> +    "bug_numbers": [1352961],
> +    "expires_in_version": "never",
> +    "kind": "exponential",
> +    "high": 10000,

From your testing, what are typical values on your machine? 10s seems a little high to me... and I don't know if there's a difference to the user if it's >5s vs >10s delay: both are interminable.

@@ +5280,5 @@
> +    "expires_in_version": "never",
> +    "kind": "exponential",
> +    "high": 10000,
> +    "n_buckets": 100,
> +    "keyed": true,

You should explain in the description what this histogram is keyed on.

(It would also be helpful to me to know what you're keying this on)

Comment 13

[Gabor Krizsanits (INACTIVE)]

Attachment: interdiff for P2 v2->v3

Comment 14

[Gabor Krizsanits (INACTIVE)]

Attachment: P2: FX_TAB_REMOTE_NAVIGATION_DELAY telemetry. v3

I've added an interdiff for easier review (see the previous comment).

Mike, I kept the waitForCondition part, we can try to resolve that in a followup but it's quite critical for e10s-multi to land this probe.

(In reply to Chris H-C :chutten from comment #12)
> > +    "alert_emails": ["[e-mail]"],
> > +    "bug_numbers": [1352961],
> > +    "expires_in_version": "never",
> > +    "kind": "exponential",
> > +    "high": 10000,
> 
> From your testing, what are typical values on your machine? 10s seems a
> little high to me... and I don't know if there's a difference to the user if
> it's >5s vs >10s delay: both are interminable.

On my machine it's around 200-300ms which is... really bad. And add-ons have the ability to make it a lot worse by adding slow process scripts...

You are right 10s is extreme, I changed that to 4s. The reason for the 10s is that on a non optimized debug build this can be seconds... and since I'm using exponential scale it should not matter much (5s-10s interval was about the last 10% of the scale on the histogram-simulator).

Comment 15

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8857930 [details] [diff] [review]
P2: FX_TAB_REMOTE_NAVIGATION_DELAY telemetry. v3

I hope the comment in the json file is enough to look at for data review, but let me know if forgot something.

Comment 16

[Chris H-C :chutten]

Comment on attachment 8857930 [details] [diff] [review]
P2: FX_TAB_REMOTE_NAVIGATION_DELAY telemetry. v3

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

Telemetry looks good. Thank you for the clarification in the commit msg.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8857930 [details] [diff] [review]
P2: FX_TAB_REMOTE_NAVIGATION_DELAY telemetry. v3

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

Thanks gabor - see below.

::: dom/ipc/tests/browser.ini
@@ +6,5 @@
>  [browser_domainPolicy.js]
>  [browser_memory_distribution_telemetry.js]
>  skip-if = !e10 # This is an e10s only probe.
> +[browser_remote_navigation_delay_telemetry.js]
> +skip-if = !e10 # This is an e10s only probe.

Still typo: this needs to be !e10s. Actually, same with the above one too. Yikes - I wonder if that test has been accidentally running for a while now...

::: dom/ipc/tests/browser_remote_navigation_delay_telemetry.js
@@ +4,5 @@
> +
> +add_task(function* test_memory_distribution() {
> +  if (Services.prefs.getIntPref("dom.ipc.processCount", 1) < 2) {
> +    ok(true, "Skip this test if e10s-multi is disabled.");
> +    finish();

You still don't need this - just return.

@@ +9,5 @@
> +    return;
> +  }
> +
> +  yield SpecialPowers.pushPrefEnv({set: [["toolkit.telemetry.enabled", true]]});
> +  Services.telemetry.canRecordExtended = true;

You should revert this to the original value in a registerCleanupFunction.

@@ +28,5 @@
> +  yield BrowserTestUtils.loadURI(tab2.linkedBrowser, "http://example.com");
> +  ok(tab2.linkedBrowser.isRemoteBrowser, "|tab2| should have a remote browser by now.");
> +
> +  // There is no good way to make sure that the parent received the histogram entries from the child processes.
> +  // Let's stick to the ugly, spinning the event loop until we're good approach.

"until we have a good approach", and perhaps reference a bug that you should file to add a better approach.

Comment 18

[Gabor Krizsanits (INACTIVE)]

(In reply to Mike Conley (:mconley) from comment #17)
> >  skip-if = !e10 # This is an e10s only probe.
> > +[browser_remote_navigation_delay_telemetry.js]
> > +skip-if = !e10 # This is an e10s only probe.
> 
> Still typo: this needs to be !e10s. Actually, same with the above one too.
> Yikes - I wonder if that test has been accidentally running for a while
> now...

Yeah... I fixed them both... Seems like I have added that changeset to the other patch
that cleans up the same things in that other test you pointed out in this one :(

Comment 19

[Gabor Krizsanits (INACTIVE)]

(In reply to Mike Conley (:mconley) from comment #17)
 > "until we have a good approach", and perhaps reference a bug that you should
> file to add a better approach.

Bug 1357509.

Comment 20

[Benjamin Smedberg]

Comment on attachment 8857930 [details] [diff] [review]
P2: FX_TAB_REMOTE_NAVIGATION_DELAY telemetry. v3

>Opening pages in a new tab might suffer an extra delay from e10s-multi because

nit, commit message should start with the bug#

>diff --git a/toolkit/components/telemetry/Histograms.json b/toolkit/components/telemetry/Histograms.json

>+  "FX_TAB_REMOTE_NAVIGATION_DELAY_MS": {
>+    "alert_emails": ["[e-mail]"],

This needs to at least have a real person/module owner attached to it. Who is responsible for this metric?

Also, although I don't mind also having a list, do we know who is on this list?

>+    "bug_numbers": [1352961],
>+    "expires_in_version": "never",

This should start out expiring (6 months/5 releases) until you have evidence it's useful, and then you can make it 'never' if there's a long-term monitoring plan.

Comment 21

[Gabor Krizsanits (INACTIVE)]

Attachment: P2: FX_TAB_REMOTE_NAVIGATION_DELAY telemetry. v4

(In reply to Benjamin Smedberg [:bsmedberg] from comment #20)
> This needs to at least have a real person/module owner attached to it. Who
> is responsible for this metric?

I've added Mike and myself.

> 
> Also, although I don't mind also having a list, do we know who is on this
> list?

I'm not sure, I was under the impression that this list is used by default. It's probably better if it alerts me directly, I should be the person responsible for this probe. Or if it's not e10s-multi related regression Mike is probably the best person to be talked to first.

> 
> >+    "bug_numbers": [1352961],
> >+    "expires_in_version": "never",
> 
> This should start out expiring (6 months/5 releases) until you have evidence
> it's useful, and then you can make it 'never' if there's a long-term
> monitoring plan.

Let's go with 61 then.

Comment 22

[Benjamin Smedberg]

Comment on attachment 8859480 [details] [diff] [review]
P2: FX_TAB_REMOTE_NAVIGATION_DELAY telemetry. v4

data-r=me

Comment 23

[Gabor Krizsanits (INACTIVE)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18520d0cbae332bcf133c769639cbf001c56e881

Comment 24

[Gabor Krizsanits (INACTIVE)]

It's adorable how we don't have a way to measure time across process boundaries but we don't allow standard libraries to be used such as chrono. I'm giving it up for the first cycle of the beta experiment.


TEST-UNEXPECTED-FAIL | check_stdcxx | We do not want these libstdc++ symbol versions to be used:
[task 2017-04-20T07:18:38.328700Z] 07:18:38     INFO -    _ZNSt6chrono3_V212system_clock3nowEv@GLIBCXX_3.4.19

Comment 25

[Gabor Krizsanits (INACTIVE)]

(In reply to Gabor Krizsanits [:krizsa :gabor] (PTO: 19-24) from comment #24)
> It's adorable how we don't have a way to measure time across process
> boundaries but we don't allow standard libraries to be used such as chrono.
> I'm giving it up for the first cycle of the beta experiment.
> 
> 
> TEST-UNEXPECTED-FAIL | check_stdcxx | We do not want these libstdc++ symbol
> versions to be used:
> [task 2017-04-20T07:18:38.328700Z] 07:18:38     INFO -   
> _ZNSt6chrono3_V212system_clock3nowEv@GLIBCXX_3.4.19

Blake, I won't be able to land this before next week, so I think we will have to run the first round of the beta experiment without this probe.

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

I think that's OK. Let's aim for sometime next week.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0605700609a5
P1: Exposing a timer to JS that can be used across the process boundaries. r=chutten
https://hg.mozilla.org/integration/mozilla-inbound/rev/9af60861b250
P2: Measure for the delay before a new tab starts processing the first URL. r=mconley, data-r=bsmedberg

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0605700609a5
https://hg.mozilla.org/mozilla-central/rev/9af60861b250

Comment 29

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8857016 [details] [diff] [review]
P1: exposing msSystemNow to have a clock that can be used across the process boundaries. v1

Approval Request Comment
[Feature/Bug causing the regression]: Not a regression.

[User impact if declined]: Will not have an important metric for the e10s-multi release.

[Is this code covered by automated tests?]: Yes.

[Has the fix been verified in Nightly?]: Yes, it's on nightly for a while and the incoming data seems correct.

[Needs manual test from QE? If yes, steps to reproduce]: No.
 
[List of other uplifts needed for the feature/fix]: This and the next patch in this bug.

[Is the change risky?]: No, I'm not aware of any risk.

[Why is the change risky/not risky?]: It's just a probe and it's well tested in practice by now.

[String changes made/needed]: None.

Comment 30

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8859480 [details] [diff] [review]
P2: FX_TAB_REMOTE_NAVIGATION_DELAY telemetry. v4

Approval Request Comment
Same as the patch above. The first part is just a helper for measuring time, this one is the actual probe with test.

Comment 31

[Gerry Chang [:gchang]]

Comment on attachment 8857016 [details] [diff] [review]
P1: exposing msSystemNow to have a clock that can be used across the process boundaries. v1

Support e10s-multi to get metric. Beta54+. Should be in 54 beta 5.

Comment 32

[Carsten Book [:Tomcat]]

needs a rebase for beta for at least part 2 

grafting 413877:9af60861b250 "Bug 1352961 - P2: Measure for the delay before a new tab starts processing the first URL. r=mconley, data-r=bsmedberg"
merging browser/components/sessionstore/SessionStore.jsm
merging browser/components/sessionstore/content/content-sessionStore.js
merging toolkit/components/telemetry/Histograms.json
warning: conflicts while merging browser/components/sessionstore/SessionStore.jsm! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c17adbc6603d
https://hg.mozilla.org/releases/mozilla-beta/rev/dca1d3b2bbb4

Comment 34

[Gabor Krizsanits (INACTIVE)]

Thanks Ryan!

Comment 35

[Andrei Vaida [:avaida]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #29)
> [Is this code covered by automated tests?]: Yes.
> 
> [Has the fix been verified in Nightly?]: Yes, it's on nightly for a while
> and the incoming data seems correct.
> 
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Gabor's assessment on manual testing needs and the fact that this fix has automated coverage.