Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1066003007:
Include application ID in log entries. (Closed)
DescriptionInclude application ID in log entries.
BUG=b/19609728
Committed: https://crrev.com/02bd8efdccb0646159c046a6ab0de5a38cbe3f62
Cr-Commit-Position: refs/heads/master@{#327338}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Use chrome.runtime.id to obtain application id information. #Patch Set 3 : Rebase to pick up latest changes. #
Messages
Total messages: 20 (9 generated)
Patchset #2 (id:20001) has been deleted
anandc@chromium.org changed reviewers: + jamiewalch@chromium.org
Jamie, PTAL. Thank you. https://codereview.chromium.org/1066003007/diff/1/remoting/webapp/crd/js/log_... File remoting/webapp/crd/js/log_to_server.js (right): https://codereview.chromium.org/1066003007/diff/1/remoting/webapp/crd/js/log_... remoting/webapp/crd/js/log_to_server.js:25: this.applicationId_ = ''; There are a few different ways we could track the app-ID information. It could either be part of the logToServer object for a session, which seems the right place for it. It could also be part of the dictionary in serverLogEntry, similar to the way the browser-version and webapp-version are. I've placed it in logToServer, because if it is in serverLogEntry, we'd have to obtain its value every time a new entry is created. We do this already for the the browser-version (https://code.google.com/p/chromium/codesearch#chromium/src/remoting/webapp/cr...), and that seems inefficient. https://codereview.chromium.org/1066003007/diff/1/remoting/webapp/crd/js/log_... remoting/webapp/crd/js/log_to_server.js:98: this.log_(entry); Is there a reason we're not setting the webapp and browser version for this log entry, like we do in logClientSessionStateChange and logAccumulatedStatistics_? https://codereview.chromium.org/1066003007/diff/1/remoting/webapp/crd/js/log_... remoting/webapp/crd/js/log_to_server.js:237: var entry = remoting.ServerLogEntry.makeSessionIdOld(this.sessionId_); Same question as above: why don't we set the webapp and browser versions for this log-entry?
https://codereview.chromium.org/1066003007/diff/1/remoting/webapp/crd/js/log_... File remoting/webapp/crd/js/log_to_server.js (right): https://codereview.chromium.org/1066003007/diff/1/remoting/webapp/crd/js/log_... remoting/webapp/crd/js/log_to_server.js:25: this.applicationId_ = ''; On 2015/04/23 00:38:32, anandc wrote: > There are a few different ways we could track the app-ID information. It could > either be part of the logToServer object for a session, which seems the right > place for it. It could also be part of the dictionary in serverLogEntry, similar > to the way the browser-version and webapp-version are. > I've placed it in logToServer, because if it is in serverLogEntry, we'd have to > obtain its value every time a new entry is created. We do this already for the > the browser-version > (https://code.google.com/p/chromium/codesearch#chromium/src/remoting/webapp/cr...), > and that seems inefficient. With my suggested change it's just a property access, so there's no efficiency gain from precomputing it. https://codereview.chromium.org/1066003007/diff/1/remoting/webapp/crd/js/log_... remoting/webapp/crd/js/log_to_server.js:98: this.log_(entry); On 2015/04/23 00:38:32, anandc wrote: > Is there a reason we're not setting the webapp and browser version for this log > entry, like we do in logClientSessionStateChange and logAccumulatedStatistics_? No; I just didn't think to include that information. I had a very particular use-case in mind for this log information, and those fields are not interesting for that case, but they should probably have been included. TBH, this class is starting to show its age, and could probably stand to be refactored so that these common fields are added automatically. https://codereview.chromium.org/1066003007/diff/1/remoting/webapp/crd/js/log_... remoting/webapp/crd/js/log_to_server.js:211: this.applicationId_ = chrome.i18n.getMessage('@@extension_id'); chrome.runtime.id would be a better way of getting this.
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/1066003007/diff/1/remoting/webapp/crd/js/log_... File remoting/webapp/crd/js/log_to_server.js (right): https://codereview.chromium.org/1066003007/diff/1/remoting/webapp/crd/js/log_... remoting/webapp/crd/js/log_to_server.js:211: this.applicationId_ = chrome.i18n.getMessage('@@extension_id'); On 2015/04/23 00:54:38, Jamie wrote: > chrome.runtime.id would be a better way of getting this. Done.
drive-by https://codereview.chromium.org/1066003007/diff/1/remoting/webapp/crd/js/log_... File remoting/webapp/crd/js/log_to_server.js (right): https://codereview.chromium.org/1066003007/diff/1/remoting/webapp/crd/js/log_... remoting/webapp/crd/js/log_to_server.js:211: this.applicationId_ = chrome.i18n.getMessage('@@extension_id'); On 2015/04/23 01:26:59, anandc wrote: > On 2015/04/23 00:54:38, Jamie wrote: > > chrome.runtime.id would be a better way of getting this. > > Done. Drive-by: For app remoting, there are cases (e.g. dev/test/staging app compiled locally), where the chrome extensionId will just be the test app's default extensionId, and the appId will be overriden via remoting.settings.getAppRemotingApplicationId() . I'm not sure which one we should use in that case - always using the extension ID will ensure that we don't pollute the production logs with test entries. Using the override application ID will ensure that we track each app separately, regardless of environment.
On 2015/04/23 01:55:51, rmsousa wrote: > drive-by > > https://codereview.chromium.org/1066003007/diff/1/remoting/webapp/crd/js/log_... > File remoting/webapp/crd/js/log_to_server.js (right): > > https://codereview.chromium.org/1066003007/diff/1/remoting/webapp/crd/js/log_... > remoting/webapp/crd/js/log_to_server.js:211: this.applicationId_ = > chrome.i18n.getMessage('@@extension_id'); > On 2015/04/23 01:26:59, anandc wrote: > > On 2015/04/23 00:54:38, Jamie wrote: > > > chrome.runtime.id would be a better way of getting this. > > > > Done. > > Drive-by: > > For app remoting, there are cases (e.g. dev/test/staging app compiled locally), > where the chrome extensionId will just be the test app's default extensionId, > and the appId will be overriden via > remoting.settings.getAppRemotingApplicationId() . > > I'm not sure which one we should use in that case - always using the extension > ID will ensure that we don't pollute the production logs with test entries. > Using the override application ID will ensure that we track each app separately, > regardless of environment. Going to create a patch-set with logging for only app-remoting. Not polluting the production logs with test entries seems to be the better option. One utility of getting logs with the test appID also included is to follow-up on any test-issues, or perhaps get performance numbers for tests. However there are better options for these scenarios, so using the chrome extension ID seems preferable.
lgtm
Patchset #3 (id:70001) has been deleted
The CQ bit was checked by anandc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1066003007/10003
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #3 (id:90001) has been deleted
The CQ bit was checked by anandc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamiewalch@chromium.org Link to the patchset: https://codereview.chromium.org/1066003007/#ps110001 (title: "Rebase to pick up latest changes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1066003007/110001
Message was sent while issue was closed.
Committed patchset #3 (id:110001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/02bd8efdccb0646159c046a6ab0de5a38cbe3f62 Cr-Commit-Position: refs/heads/master@{#327338} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
