Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
81603002:
Send UMA stability stats in a separate UMA log on startup. (Closed)
|
Created:
7 years, 1 month ago by Alexei Svitkine (slow) Modified:
7 years ago CC:
chromium-reviews, Ilya Sherman, jar (doing other things), asvitkine+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionSend UMA stability stats in a separate UMA log on startup.
This is implemented behind a field trial, so that we can roll-out this
functionality while monitoring crash stats, to ensure there's no problem
with the stats being misreported.
This change splits the initial UMA upload into two separate logs when the
previous session didn't end cleanly. The first will contain the system profile
from the previous session (which has been saved to local state), as well as
the Chrome stability stats. The second log will have the regular startup
metrics (histograms and profiler data).
With the new (behind a field-trial) behavior, the code also reads unsent
logs from prefs (that are already in memory) earlier, so that it can persist
the initial stability log, in case a crash happens before it's sent. I've
added some UMA histograms to track how long reading/saving unsent logs
takes and will evaluate this when ramping up the field trial to ensure the
time taken is acceptable.
BUG=312733
TEST=Manual by running with command-line --enable-metrics-reporting-
for-testing --force-fieldtrials=UMAStability/SeparateLog/ and running my
own modified UMA server and logging requests. Also, new unit tests.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240919
Patch Set 1 : #
Total comments: 38
Patch Set 2 : #Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #Patch Set 6 : #
Total comments: 10
Patch Set 7 : #
Total comments: 12
Patch Set 8 : #Patch Set 9 : Fixed tests on Windows and Linux. #Patch Set 10 : #Patch Set 11 : #
Messages
Total messages: 30 (0 generated)
Hey Ilya, Can you take an initial look? I've tested this manually with my own UMA server and it seems to work as expected, though I've not been able to find a good way to write unit tests for this. :\ It's also a bit more complex than I'd hoped for, partly due to the fact that I'm doing this as a field trial which required some extra refactoring to the init sequence and keeping some additional logic (to preserve the previous behaviour when the field trial is not on). Let me know what you think and we can iterate on this. Once we're on the same page, we can add Jim as a reviewer, as well.
Sorry, didn't get to this today. I'll try to prioritize this review tomorrow.
https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.cc (left): https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1472: DCHECK_EQ(INIT_TASK_DONE, state_); nit: Please preserve some flavor of this DCHECK. https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:80: // INIT_TASK_SCHEDULED, // Waiting for deferred init tasks to finish. nit: Note that you still have "tasks" here, but you've switched to "task" below. IMO you should keep "tasks" throughout. https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:108: // This state will be entered if there's such a stability log waiting to be nit: What does it mean for there to be a stability log waiting to be transmitted? It might be clearer to reference when (during which step?) such a log would be created. https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:109: // transmitted when its time to sent up the first log (per the reporting nit: "its" -> "it's" https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:109: // transmitted when its time to sent up the first log (per the reporting nit: "sent" -> "send" https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:117: // any. It is also the case that any previously unsent logs have been loaded nit: This first sentence is a bit hard to parse; consider rewording. https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:947: DCHECK_EQ(0, startup_uptime.InMicroseconds()); Hmm, why does this need to happen earlier now? https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1531: // which is currently done in PrepareInitialMetricsLog(). I think it's important to get this right, i.e. to save the stability log to prefs in case of a crash and to load the existing unsent logs earlier (so we don't accidentally overwrite them with the wrong data). https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1536: DCHECK(initial_metrics_log_.get()); nit: No need for .get() https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1557: log_manager_.LoadPersistedUnsentLogs(); nit: It seems more appropriate to leave this where it was. This is not really related to preparing the initial metrics log. https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1708: // don't consider that a sign that the server is in trouble. nit: This comment still belongs above line 1702 IMO. https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.h:122: void InitializeMetricsRecordingState(bool reporting_will_be_enabled); nit: Please define an enum to use in place of the boolean, since the meaning of "true" or "false" is currently not obvious at call sites. https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.h:122: void InitializeMetricsRecordingState(bool reporting_will_be_enabled); Why is this method public? It doesn't look like you added any clients of this code as part of this CL. https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.h:264: INIT_TASK_SCHEDULED, // Waiting for deferred init task to finish. nit: "tasks" seems more accurate, since this can be waiting on any of a number of steps. https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.h:267: SENDING_INITIAL_METRICS_LOG, // Initial metrics log being sent. Jim made a good point in a previous discussion that there might not be much reason anymore to distinguish between the initial log and ongoing logs, other than stability logs. Is the plan to get rid of or otherwise rename this state once the field trial is at 100%? https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.h:405: // a previous session and metrics stability stats and stages it for upload. nit: "and metrics stability stats" -> rephrase somehow? Maybe try to clarify that this *only* uploads data coming from a previous session.
https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.cc (left): https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1472: DCHECK_EQ(INIT_TASK_DONE, state_); On 2013/12/06 06:21:26, Ilya Sherman wrote: > nit: Please preserve some flavor of this DCHECK. Done (in PrepareInitialMetricsLog()). https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:80: // INIT_TASK_SCHEDULED, // Waiting for deferred init tasks to finish. On 2013/12/06 06:21:26, Ilya Sherman wrote: > nit: Note that you still have "tasks" here, but you've switched to "task" below. > IMO you should keep "tasks" throughout. Done. https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:108: // This state will be entered if there's such a stability log waiting to be On 2013/12/06 06:21:26, Ilya Sherman wrote: > nit: What does it mean for there to be a stability log waiting to be > transmitted? It might be clearer to reference when (during which step?) such a > log would be created. Done. https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:109: // transmitted when its time to sent up the first log (per the reporting On 2013/12/06 06:21:26, Ilya Sherman wrote: > nit: "sent" -> "send" Done. https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:109: // transmitted when its time to sent up the first log (per the reporting On 2013/12/06 06:21:26, Ilya Sherman wrote: > nit: "its" -> "it's" Done. https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:117: // any. It is also the case that any previously unsent logs have been loaded On 2013/12/06 06:21:26, Ilya Sherman wrote: > nit: This first sentence is a bit hard to parse; consider rewording. Done. https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:947: DCHECK_EQ(0, startup_uptime.InMicroseconds()); On 2013/12/06 06:21:26, Ilya Sherman wrote: > Hmm, why does this need to happen earlier now? In a previous version of this I used startup_uptime for the initial stability log, so it was needed earlier. But now I just pass 0 unconditionally, so this doesn't need to be here anymore - you're right. Moved back down. https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1531: // which is currently done in PrepareInitialMetricsLog(). On 2013/12/06 06:21:26, Ilya Sherman wrote: > I think it's important to get this right, i.e. to save the stability log to > prefs in case of a crash and to load the existing unsent logs earlier (so we > don't accidentally overwrite them with the wrong data). Okay - that's why I put the comment here to raise this point so we can discuss. :) In order to handle this, we will need to read the existing unsent logs from prefs earlier (i.e. in this function by moving the log_manager_.LoadPersistedUnsentLogs() call here). The consequence of this is it will now be on the startup path. The current code calls LoadPersistedUnsentLogs() once the initial metrics log is ready to be sent up - quite a bit later. Whereas this function is executed shortly after metrics service creation while we're in single-threaded startup. I'm not sure if we're willing to do this. One thing to note is the unsent logs are already in memory (i.e. local state has been read and that call wouldn't result in any I/O) - so its just the cpu load of decoding the logs and verifying their hashes, etc. So it may be okay to do. WDYT? The reason we need to read them is so that when we serialize this out, it doesn't clobber existing logs. One possibility would be to add to the existing list without reading the existing contents out. This requires extra support for this through the log manager (right now it only knows how to clobber the full list). The extra support required would be to be able to append a log to a list (which would be done here.) and another function to remove it (done when the log has been sent successfully). Perhaps this is the best approach (despite requiring more extensive changes). WDYT? https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1536: DCHECK(initial_metrics_log_.get()); On 2013/12/06 06:21:26, Ilya Sherman wrote: > nit: No need for .get() I actually just deleted this line now. If it's null, then the line below will crash anyway, so no need an extra DCHECK here. (DCHECKs are useful for catching things early - e.g. if this didn't use the log until much later - but doing it right above a deref doesn't serve any value - either way the error will point at this part of the code). https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1557: log_manager_.LoadPersistedUnsentLogs(); On 2013/12/06 06:21:26, Ilya Sherman wrote: > nit: It seems more appropriate to leave this where it was. This is not really > related to preparing the initial metrics log. See my comment above about moving this call earlier. https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1708: // don't consider that a sign that the server is in trouble. On 2013/12/06 06:21:26, Ilya Sherman wrote: > nit: This comment still belongs above line 1702 IMO. Done. https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.h:122: void InitializeMetricsRecordingState(bool reporting_will_be_enabled); On 2013/12/06 06:21:26, Ilya Sherman wrote: > Why is this method public? It doesn't look like you added any clients of this > code as part of this CL. Yes, that was my mistake - I forgot to include the corresponding changes to chrome_browser_main.cc as part of the original CL - fixed now. The reason this is public is so that it can be called after field trials are created (so that its behavior can be based on field trial state). https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.h:122: void InitializeMetricsRecordingState(bool reporting_will_be_enabled); On 2013/12/06 06:21:26, Ilya Sherman wrote: > nit: Please define an enum to use in place of the boolean, since the meaning of > "true" or "false" is currently not obvious at call sites. Done. https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.h:264: INIT_TASK_SCHEDULED, // Waiting for deferred init task to finish. On 2013/12/06 06:21:26, Ilya Sherman wrote: > nit: "tasks" seems more accurate, since this can be waiting on any of a number > of steps. Unfortunately, it goes above the 80 col limit (if we want to keep the //'s aligned). We could change the comments to be before each enum, WDYT? https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.h:267: SENDING_INITIAL_METRICS_LOG, // Initial metrics log being sent. On 2013/12/06 06:21:26, Ilya Sherman wrote: > Jim made a good point in a previous discussion that there might not be much > reason anymore to distinguish between the initial log and ongoing logs, other > than stability logs. Is the plan to get rid of or otherwise rename this state > once the field trial is at 100%? The code already treats it as an ongoing log (semantically) when the field trial is on. However, it is special still - for example it has profiler data and doesn't have counts, so having a special name "initial metrics log" still makes sense as the current code stands. However, long term I agree we should try to make it similar to regular ongoing logs - but we'd probably want to figure out how we want to log profiler data in the future before that happens, for example. So I'd leave it as is. https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.h:405: // a previous session and metrics stability stats and stages it for upload. On 2013/12/06 06:21:26, Ilya Sherman wrote: > nit: "and metrics stability stats" -> rephrase somehow? Maybe try to clarify > that this *only* uploads data coming from a previous session. Done.
https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1531: // which is currently done in PrepareInitialMetricsLog(). On 2013/12/06 17:59:22, Alexei Svitkine wrote: > On 2013/12/06 06:21:26, Ilya Sherman wrote: > > I think it's important to get this right, i.e. to save the stability log to > > prefs in case of a crash and to load the existing unsent logs earlier (so we > > don't accidentally overwrite them with the wrong data). > > Okay - that's why I put the comment here to raise this point so we can discuss. > :) > > In order to handle this, we will need to read the existing unsent logs from > prefs earlier (i.e. in this function by moving the > log_manager_.LoadPersistedUnsentLogs() call here). > > The consequence of this is it will now be on the startup path. The current code > calls LoadPersistedUnsentLogs() once the initial metrics log is ready to be sent > up - quite a bit later. Whereas this function is executed shortly after metrics > service creation while we're in single-threaded startup. I'm not sure if we're > willing to do this. > > One thing to note is the unsent logs are already in memory (i.e. local state has > been read and that call wouldn't result in any I/O) - so its just the cpu load > of decoding the logs and verifying their hashes, etc. So it may be okay to do. > WDYT? Hmm. Could you possibly measure the actual performance impact of this change? It's hard to say whether this is reasonable without any data. If the logs are already in memory, though, my intuition is that reading them ought to be fairly cheap. > The reason we need to read them is so that when we serialize this out, it > doesn't clobber existing logs. One possibility would be to add to the existing > list without reading the existing contents out. This requires extra support for > this through the log manager (right now it only knows how to clobber the full > list). The extra support required would be to be able to append a log to a list > (which would be done here.) and another function to remove it (done when the log > has been sent successfully). Perhaps this is the best approach (despite > requiring more extensive changes). WDYT? Depends on how much extra complexity that is, I guess. Yet another option is to delay the stability metrics log until we're out of the startup path. What are the tradeoffs you're thinking of there? https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.h:264: INIT_TASK_SCHEDULED, // Waiting for deferred init task to finish. On 2013/12/06 17:59:22, Alexei Svitkine wrote: > On 2013/12/06 06:21:26, Ilya Sherman wrote: > > nit: "tasks" seems more accurate, since this can be waiting on any of a number > > of steps. > > Unfortunately, it goes above the 80 col limit (if we want to keep the //'s > aligned). We could change the comments to be before each enum, WDYT? You can wrap the line like so: INIT_TASK_SCHEDULED, // Waiting for deferred init tasks to // complete. https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.h:267: SENDING_INITIAL_METRICS_LOG, // Initial metrics log being sent. On 2013/12/06 17:59:22, Alexei Svitkine wrote: > On 2013/12/06 06:21:26, Ilya Sherman wrote: > > Jim made a good point in a previous discussion that there might not be much > > reason anymore to distinguish between the initial log and ongoing logs, other > > than stability logs. Is the plan to get rid of or otherwise rename this state > > once the field trial is at 100%? > > The code already treats it as an ongoing log (semantically) when the field trial > is on. However, it is special still - for example it has profiler data and > doesn't have counts, so having a special name "initial metrics log" still makes > sense as the current code stands. However, long term I agree we should try to > make it similar to regular ongoing logs - but we'd probably want to figure out > how we want to log profiler data in the future before that happens, for example. > So I'd leave it as is. Ok. https://codereview.chromium.org/81603002/diff/550001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/81603002/diff/550001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.h:411: // upload. nit: Sorry to super nitpick, but I still find this comment not entirely clear. A suggested rephrasing, feel free to further refine: "Prepares the initial stability log, which is only logged when the previous run of Chrome crashed. This log contains any stability metrics left over from that previous run, and only these stability metrics. It uses the system profile from the previous session."
https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1531: // which is currently done in PrepareInitialMetricsLog(). On 2013/12/07 06:51:53, Ilya Sherman wrote: > Hmm. Could you possibly measure the actual performance impact of this change? > It's hard to say whether this is reasonable without any data. If the logs are > already in memory, though, my intuition is that reading them ought to be fairly > cheap. Agreed. I've added histograms to measure how long it takes to load and store unsent logs and made the new code path (that's behind a field trial) do this more eagerly now, allowing the initial stability log to be persisted. I figure given that this is all behind a trial, we can look at the metrics and decide if its acceptable to ramp things up given what we see. WDYT? > Depends on how much extra complexity that is, I guess. > > Yet another option is to delay the stability metrics log until we're out of the > startup path. What are the tradeoffs you're thinking of there? The problem is if we delay the stability metrics, then stuff from the current session (i.e. new renderer crashes) will be recorded in that log, which is incorrect. https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.h:264: INIT_TASK_SCHEDULED, // Waiting for deferred init task to finish. On 2013/12/07 06:51:53, Ilya Sherman wrote: > On 2013/12/06 17:59:22, Alexei Svitkine wrote: > > On 2013/12/06 06:21:26, Ilya Sherman wrote: > > > nit: "tasks" seems more accurate, since this can be waiting on any of a > number > > > of steps. > > > > Unfortunately, it goes above the 80 col limit (if we want to keep the //'s > > aligned). We could change the comments to be before each enum, WDYT? > > You can wrap the line like so: > > INIT_TASK_SCHEDULED, // Waiting for deferred init tasks to > // complete. Done. https://codereview.chromium.org/81603002/diff/550001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/81603002/diff/550001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.h:411: // upload. On 2013/12/07 06:51:54, Ilya Sherman wrote: > nit: Sorry to super nitpick, but I still find this comment not entirely clear. > A suggested rephrasing, feel free to further refine: > > "Prepares the initial stability log, which is only logged when the previous run > of Chrome crashed. This log contains any stability metrics left over from that > previous run, and only these stability metrics. It uses the system profile from > the previous session." Done.
Thanks, this looks pretty reasonable to me. I think it might be time to loop in Jim as well :) https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/81603002/diff/510001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1531: // which is currently done in PrepareInitialMetricsLog(). On 2013/12/09 21:17:02, Alexei Svitkine wrote: > On 2013/12/07 06:51:53, Ilya Sherman wrote: > > Hmm. Could you possibly measure the actual performance impact of this change? > > > It's hard to say whether this is reasonable without any data. If the logs are > > already in memory, though, my intuition is that reading them ought to be > fairly > > cheap. > > Agreed. I've added histograms to measure how long it takes to load and store > unsent logs and made the new code path (that's behind a field trial) do this > more eagerly now, allowing the initial stability log to be persisted. I figure > given that this is all behind a trial, we can look at the metrics and decide if > its acceptable to ramp things up given what we see. WDYT? Yeah, that sounds reasonable. Thanks! :) > > Depends on how much extra complexity that is, I guess. > > > > Yet another option is to delay the stability metrics log until we're out of > the > > startup path. What are the tradeoffs you're thinking of there? > > The problem is if we delay the stability metrics, then stuff from the current > session (i.e. new renderer crashes) will be recorded in that log, which is > incorrect. Makes sense. https://codereview.chromium.org/81603002/diff/690001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/81603002/diff/690001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1542: log_manager_.PersistUnsentLogs(); nit: Call StoreUnsentLogs() here instead? https://codereview.chromium.org/81603002/diff/690001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1683: case SENDING_INITIAL_STABILITY_LOG: Seems like you should StoreUnsentLogs() here, just as is done in SENDING_OLD_LOGS, so that we don't accidentally try to send stability logs multiple times.
+Jim Hey Jim, could you take a look? This is the change I've alluded to in my previous CL that changes MetricsLog to save off the system profile. I've already iterated a bit on this with Ilya (see previous discussion) and we feel it's ready for you to take a look now as well. The plan is to have a field trial that enables the new behavior which can be ramped up from the server side while we monitor metrics (e.g. stability numbers, etc) to ensure there's no issues with the new approach. As such, the code is a little bit more complex than if the new behavior is adopted wholesale, as I've kept the old functionality for the non-field-trial behavior. https://codereview.chromium.org/81603002/diff/690001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/81603002/diff/690001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1542: log_manager_.PersistUnsentLogs(); On 2013/12/10 06:02:10, Ilya Sherman wrote: > nit: Call StoreUnsentLogs() here instead? Done. I removed the |state_| check in that function, since this is loading the logs earlier than what the function was checking for. However, the goal of that check was to ensure that we don't write logs before reading existing ones (so that they don't get clobbered). To preserve that guarantee, I've modified MetricsLogManager to do that check, rather than doing that in MetricsService. (I've also had to modify some MetricsLogManager tests to not trigger that check). https://codereview.chromium.org/81603002/diff/690001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1683: case SENDING_INITIAL_STABILITY_LOG: On 2013/12/10 06:02:10, Ilya Sherman wrote: > Seems like you should StoreUnsentLogs() here, just as is done in > SENDING_OLD_LOGS, so that we don't accidentally try to send stability logs > multiple times. Good point. Done.
I've now added a couple of basic tests for the new functionality, which is better than none. ;) Jim, friendly ping on the review.
https://codereview.chromium.org/81603002/diff/750001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/81603002/diff/750001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1543: log_manager_.PersistUnsentLogs(); It looks like you're now persisting unsent logs twice. Am I missing something? https://codereview.chromium.org/81603002/diff/750001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1574: } nit: This method seems unnecessary now that it doesn't have the DCHECK. WDYT of just removing it instead? https://codereview.chromium.org/81603002/diff/750001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1689: case SENDING_INITIAL_METRICS_LOG: Seems like this case could also use a StoreUnsentLogs() -- am I missing a reason why it's not needed? Also, could you please confirm for me -- ideally via a link to the relevant code -- that the initial metrics log gets inserted into the queue of logs that would be saved to disk? https://codereview.chromium.org/81603002/diff/750001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service_unittest.cc (right): https://codereview.chromium.org/81603002/diff/750001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service_unittest.cc:200: EXPECT_TRUE(service.log_manager().has_unsent_logs()); Can you check the contents of the unsent logs as well?
https://codereview.chromium.org/81603002/diff/750001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/81603002/diff/750001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1543: log_manager_.PersistUnsentLogs(); On 2013/12/12 09:03:38, Ilya Sherman wrote: > It looks like you're now persisting unsent logs twice. Am I missing something? Good catch - I forgot to remove the other call. Done. https://codereview.chromium.org/81603002/diff/750001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1574: } On 2013/12/12 09:03:38, Ilya Sherman wrote: > nit: This method seems unnecessary now that it doesn't have the DCHECK. WDYT of > just removing it instead? Done. https://codereview.chromium.org/81603002/diff/750001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1689: case SENDING_INITIAL_METRICS_LOG: On 2013/12/12 09:03:38, Ilya Sherman wrote: > Seems like this case could also use a StoreUnsentLogs() -- am I missing a reason > why it's not needed? Also, could you please confirm for me -- ideally via a > link to the relevant code -- that the initial metrics log gets inserted into the > queue of logs that would be saved to disk? This case actually works a little differently. The initial metrics log gets added to the list when its created in PrepareInitialMetricsLog() via FinishCurrentLog(): https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/metr... But then gets immediately removed from the unsent list when it's staged PrepareInitialMetricsLog() actually stages the log, which removes it from the list of unsent logs: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/metr... So, when the log is staged (i.e. the current staged_log on the metrics log manager), its not in the unsent list. However, SendStagedLog() is called immediately after (in the old path, this happens in OnFinalLogInfoCollectionDone()). So, to summarize, the initial metrics log is not persisted in the current code, however it sent up immediately after being created - unlike the new stability log I'm adding. (As an aside, it looks like there's code to provisionally store a log while its being uploaded - i.e. via PushPendingLogsToPersistentStorage(), but from what I can tell that doesn't actually run in this case other than the mobile "app enter background" case...) I agree that this might be slightly sub-optimal, but also not completely terrible since the initial metrics log is sent up immediately after being created. I'd prefer leaving this as-is for now (so that this CL doesn't change existing functionality outside the new behaviour that's controlled by a field trial). WDYT? https://codereview.chromium.org/81603002/diff/750001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service_unittest.cc (right): https://codereview.chromium.org/81603002/diff/750001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service_unittest.cc:200: EXPECT_TRUE(service.log_manager().has_unsent_logs()); On 2013/12/12 09:03:38, Ilya Sherman wrote: > Can you check the contents of the unsent logs as well? Done.
I'm fine with Ilya doing the review.. and providing an L G T M... When he is happy... you should land. I have some inline comments below. tl;dr; <ramblings about a future> At a higher level, this whole file is a rats nets. It was a super terrible rats nest when I first entered the scene... I think I helped it a lot (yeah... it is hard to believe it could be worse, eh?)... and now it is just a rats nest which continues to grow worse over time. It is my fault for being afraid to change this central engine while we were flying (and were super-unstable, pre-release). I kept imagining we'd reach a more stable era, with more wiggle room to get the refactor right. That time will surely never come :-/. It would be a Real Nice Thing of someone went in and broke out the state machine more from the big activities... provided an easy way to to substitute in Mock activities, so that we get clearer exercise/test. I don't think it should come in this CL (which seems to serve an urgent need), but this code is super hard to read (even when I think I know what it is doing <sigh>). It is good that you're measuring log load time (from persistent store). I think this is often a large number, and worse yet, mostly done on the UI thread. I'm also afraid that we're swelling the size of the prefs file, as I'm wary that shutdown logs from the last run have grown bigger and bigger (as we've added histograms). We should probably look at a place to persist this stuff that does not impact startup (or shutdown) as much... perhaps some database service... maybe the disk-cache guys have a better service... hard to say. https://codereview.chromium.org/81603002/diff/770001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/81603002/diff/770001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:946: MetricsLog::GetBuildTime()); I'm wary of the ordering.... are you clobbering the old BuildTime before you've used it to generate old stability logs with that build time? https://codereview.chromium.org/81603002/diff/770001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:963: // If the previous session didn't exit cleanly, then prepare an initial I would have expected that you would create a stability log if there was an unsent crash count. It appears that you'll only send if the immediately preceding run crashed (an not for instance if it was merely started and shutdown quickly). Is this as desired? https://codereview.chromium.org/81603002/diff/770001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1542: log_manager_.PersistUnsentLogs(); There is always a tension between losing logs, and accidentally sending them twice. Unless we have de-duping help server side, we probably have to err in favor of losing some. Note that we already go in this direction when we have too many unsent logs. Can you describe how the above line fits in? https://codereview.chromium.org/81603002/diff/770001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/81603002/diff/770001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.h:114: enum ReportingState { nit: curious question: Why did you decide to use an enum, when a bool seem(s)(ed) like a perfect match? I usually turn to an enum if/when I find a function has a bunch of bools as arguments, and is getting unwieldy. If not, bool is often more readable etc. https://codereview.chromium.org/81603002/diff/770001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.h:407: // run of Chrome crashed. This log contains any stability metrics left over nit: consider: "...run of chrome crashed." --> ...run of Chrome has some crash(es) to report." I'm assuming that you send a stability log if you have browser or renderer crashes, and not merely browser crashes. ...but reading the code, it seems slightly different from either choice. Can you clear up the confusion (for me)?
Thanks for looking, Jim. I've replied to your comments below and I completely agree with your high-level intro comment - long term this class does need refactoring to make it more manageable and easier to test. https://codereview.chromium.org/81603002/diff/770001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/81603002/diff/770001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:946: MetricsLog::GetBuildTime()); On 2013/12/12 17:34:34, jar wrote: > I'm wary of the ordering.... are you clobbering the old BuildTime before you've > used it to generate old stability logs with that build time? If this block gets hit (version mismatch between current version and old version), then all the saved stability stats will be clobbered (including kStabilityExitedCleanly being reset as well), so the initial stability log will not be generated in this case. I've kept this block here intentionally in this CL, so as to not diverge too much from the old behavior at this stage (to make it easy to compare stats when turning on the new behavior via field trial). When we're happy with the new approach, I do plan to remove this block entirely, so that we can receive logs from previous versions as well, but I'm explicitly not doing that in this CL per the above. (And when I do remove the block, then the ordering concern goes away since there won't be any clobbering any more.) https://codereview.chromium.org/81603002/diff/770001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:963: // If the previous session didn't exit cleanly, then prepare an initial On 2013/12/12 17:34:34, jar wrote: > I would have expected that you would create a stability log if there was an > unsent crash count. It appears that you'll only send if the immediately > preceding run crashed (an not for instance if it was merely started and shutdown > quickly). > > Is this as desired? I've thought about this and still think this is okay. One thing to note is that if we don't create a new stability log here, the stability stats are not lost and will just be recorded in the next regular UMA log. So worst case scenario, these stats end up being reported in a different log. However, let's imagine when the above scenario could happen. First, observe that the only time we increment kStabilityCrashCount is in this block. Which means, the only cases where that pref can be non-zero would be either: a) users moving from old behavior to new behavior - these will disappear over time b) if they had metrics reporting disabled before and they've accumulated a lot of crashes during that time before enabling UMA I do not believe users of category a) are worth worrying about, since this is very temporary and these users will disappear once everyone switches to the new behavior. (And as I described earlier, those stats wouldn't be lost anyway, just won't be sent in the initial separate stability log.) Whereas for category b), I actually think it would be somewhat detrimental to report these crashes more aggressively. Because if they have lots of crashes saved up from a long time ago and they enable UMA, they'll suddenly report a whole lot of crashes, which may skew whatever metrics are being looked at. The other issue is that for these users, we likely wouldn't even have their system proto saved from their previous session (since they didn't have UMA enabled then), so it may not be possible to record the initial stability log anyway. Plus, since they've had many crashes saved - likely these would be from many different sessions, so even a single saved system profile proto wouldn't really be accurate. So I don't see a win from handling category b) in order to send a separate initial stability log, given the above. (And in the future, it may be worthwhile to actually clear crash stats when someone toggles UMA on, so that accumulated stats from many sessions before don't end up spiking stats - but I'd rather not do that in this CL). Let me know if the above makes sense to you, or whether you disagree with my analysis. Thanks! https://codereview.chromium.org/81603002/diff/770001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1542: log_manager_.PersistUnsentLogs(); On 2013/12/12 17:34:34, jar wrote: > There is always a tension between losing logs, and accidentally sending them > twice. > > Unless we have de-duping help server side, we probably have to err in favor of > losing some. Note that we already go in this direction when we have too many > unsent logs. > > Can you describe how the above line fits in? In this case, we're saving the newly recorded initial stability log in prefs right after we generate it (via this call). With this CL, we'll transmit this log at the same time as previously we'd send the initial metrics log. Which is 60 seconds after startup (on desktop). That's a relatively long time and a crash can definitely happen during that period. Since we've already cleared the stability stats at this point when putting them in the log (unlike the old behavior which does that 60 seconds after startup), we really don't want to lose this information - so persisting this log here makes sense. Note that when it's time to transmit this log, which as I've mentioned is 60 seconds later, then we'll call log_manager_.StageNextLogForUpload() on this log, which will remove it from the unsent logs list. However, this won't immediately serialize the unsent list back to local state. We do that once we receive the result of uploading it (but also I think periodically, which possibly could happen while the log is in transit before we get a reply). Anyway, once we get a reply and it was successful, then we'll persist the updated unsent logs (i.e. with the log that was sent up removed) back. In addition to all the above, it's worth noting that the actual writing of local state to disk also happens periodically (every 10 seconds I believe?), so even immediately saving things to local state doesn't guarantee that they're actually written. I believe the above accurately describes the behavior with this CL. If you read carefully, you can probably spot some holes in the above where we may lose logs or send them twice, but I don't think its any different than the current initial metrics log behavior (without this CL). https://codereview.chromium.org/81603002/diff/770001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/81603002/diff/770001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.h:114: enum ReportingState { On 2013/12/12 17:34:34, jar wrote: > nit: curious question: Why did you decide to use an enum, when a bool > seem(s)(ed) like a perfect match? I usually turn to an enum if/when I find a > function has a bunch of bools as arguments, and is getting unwieldy. If not, > bool is often more readable etc. I went with a bool initially, but Ilya preferred an enum (so that callsites are more descriptive). https://codereview.chromium.org/81603002/diff/770001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.h:407: // run of Chrome crashed. This log contains any stability metrics left over On 2013/12/12 17:34:34, jar wrote: > nit: consider: > "...run of chrome crashed." --> ...run of Chrome has some crash(es) to report." > > I'm assuming that you send a stability log if you have browser or renderer > crashes, and not merely browser crashes. > > ...but reading the code, it seems slightly different from either choice. Can > you clear up the confusion (for me)? I addressed this more in another comment (specifically "have any crashes" vs. "crashed last session"), but let me discuss specifically the case of renderer (and other process crashes) here. On a clean shutdown, MetricsService will produce a final UMA log that will be persisted with the stats since the last UMA log. So any renderer crashes (and other process crashes) in such a case will already have been recorded to that log (and cleared from prefs). So the only there can be renderer crashes on the next start up would be in the case of an unclean shutdown, which is consistent with what this code is doing and what this comment is describing. (I've also done some queries of the logs before deciding to go this way to ensure that the stats are consistent with the above and did find they are - there were very very few renderer crashes reported in the initial UMA log - though still some since in the current code the initial log happens after 60 seconds, so it is still possible for a renderer to crash before then.)
LGTM, thanks Alexei. https://codereview.chromium.org/81603002/diff/750001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/81603002/diff/750001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1689: case SENDING_INITIAL_METRICS_LOG: On 2013/12/12 16:09:27, Alexei Svitkine wrote: > On 2013/12/12 09:03:38, Ilya Sherman wrote: > > Seems like this case could also use a StoreUnsentLogs() -- am I missing a > reason > > why it's not needed? Also, could you please confirm for me -- ideally via a > > link to the relevant code -- that the initial metrics log gets inserted into > the > > queue of logs that would be saved to disk? > > This case actually works a little differently. > > The initial metrics log gets added to the list when its created in > PrepareInitialMetricsLog() via FinishCurrentLog(): > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/metr... > > But then gets immediately removed from the unsent list when it's staged > > PrepareInitialMetricsLog() actually stages the log, which removes it from the > list of unsent logs: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/metr... > > So, when the log is staged (i.e. the current staged_log on the metrics log > manager), its not in the unsent list. However, SendStagedLog() is called > immediately after (in the old path, this happens in > OnFinalLogInfoCollectionDone()). > > So, to summarize, the initial metrics log is not persisted in the current code, > however it sent up immediately after being created - unlike the new stability > log I'm adding. > > (As an aside, it looks like there's code to provisionally store a log while its > being uploaded - i.e. via PushPendingLogsToPersistentStorage(), but from what I > can tell that doesn't actually run in this case other than the mobile "app enter > background" case...) > > I agree that this might be slightly sub-optimal, but also not completely > terrible since the initial metrics log is sent up immediately after being > created. I'd prefer leaving this as-is for now (so that this CL doesn't change > existing functionality outside the new behaviour that's controlled by a field > trial). WDYT? Not changing existing behavior as part of this CL sounds fine. Might be good to file a bug and add a TODO to the code, though, so that this has a higher chance of getting fixed up eventually :) https://codereview.chromium.org/81603002/diff/770001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/81603002/diff/770001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1524: DCHECK_EQ(INITIALIZED, state_); nit: Could you also DCHECK that there is crash data to send up? That's currently an implicit assumption of this method, afaict.
Thanks! I'll give it a day or so in case Jim has any comments about my earlier replies and if not will hit the cq on the weekend. https://codereview.chromium.org/81603002/diff/750001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/81603002/diff/750001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1689: case SENDING_INITIAL_METRICS_LOG: On 2013/12/12 23:45:20, Ilya Sherman wrote: > Not changing existing behavior as part of this CL sounds fine. Might be good to > file a bug and add a TODO to the code, though, so that this has a higher chance > of getting fixed up eventually :) Done. Added a TODO and filed http://crbug.com/328417. https://codereview.chromium.org/81603002/diff/770001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/81603002/diff/770001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.cc:1524: DCHECK_EQ(INITIALIZED, state_); On 2013/12/12 23:45:20, Ilya Sherman wrote: > nit: Could you also DCHECK that there is crash data to send up? That's > currently an implicit assumption of this method, afaict. Done.
+thakis for chrome/browser/chrome_browser_main.cc owners
lgtm
Fixed tests on Windows and Linux.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/[email protected]/81603002/830001
Retried try job too often on linux_rel for step(s) cacheinvalidation_unittests, cc_unittests, check_deps, chromedriver2_unittests, components_unittests, content_browsertests, content_unittests, crypto_unittests, google_apis_unittests, gpu_unittests, ipc_tests, jingle_unittests, media_unittests, nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests, sandbox_linux_unittests, sql_unittests, sync_integration_tests, sync_unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/[email protected]/81603002/850001
Retried try job too often on linux_chromeos for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/[email protected]/81603002/870001
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/[email protected]/81603002/870001
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/[email protected]/81603002/870001
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/[email protected]/81603002/870001
Message was sent while issue was closed.
Change committed as 240919 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
