Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11014006:
Add Android fingerprint to metrics logs (Closed)
|
Created:
8 years, 2 months ago by gone Modified:
8 years, 2 months ago CC:
chromium-reviews, MAD, erikwright+watch_chromium.org, jar (doing other things), Ted C Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdd Android fingerprint to metrics logs
(Upstreaming)
Adds information about the Android build fingerprint to information that we
send back in our metrics.
BUG=153694
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=159957
Patch Set 1 #
Total comments: 9
Patch Set 2 : Removing XML changes #
Messages
Total messages: 17 (0 generated)
Still running through the try bots, but I had to make some adjustments to the downstream code to clean it up. Can you take a sanity check pass?
Adding isherman for more sanity checking. This is slightly different from how our tree currently looks, but it should be more correct in the long run.
More comments inline, but please note that if you integrate this change, it will require server-side changes as well. If you're already uploading this data, and merely upstreaming it here, where is it getting processed? I didn't see any corresponding server-side changes land... https://chromiumcodereview.appspot.com/11014006/diff/1/chrome/browser/metrics... File chrome/browser/metrics/metrics_log.cc (right): https://chromiumcodereview.appspot.com/11014006/diff/1/chrome/browser/metrics... chrome/browser/metrics/metrics_log.cc:335: #endif I don't think it's appropriate to add the OS info as part of recording incremental stability data. For the protocol buffer uploads, this data should already be included as part of RecordEnvironmentProto(), which is called immediately before RecordIncrementalStabilityElements() in the MetricsService. https://chromiumcodereview.appspot.com/11014006/diff/1/chrome/browser/metrics... chrome/browser/metrics/metrics_log.cc:901: // Write the XML version. As I mentioned above, I don't think you should move this code. However, if you do, you should document why this method only writes the XML version, and probably name it accordingly. https://chromiumcodereview.appspot.com/11014006/diff/1/chrome/browser/metrics... chrome/browser/metrics/metrics_log.cc:907: base::android::BuildInfo::GetInstance()->android_build_fp()); I don't recommend trying to add this field to the XML-based uploads, as that pipeline is deprecated. The only reason to add this field to the XML uploads is if you need it like yesterday for M18-based Android builds... but then there's not much point to upstreaming the change to the main chrome/ repository. https://chromiumcodereview.appspot.com/11014006/diff/1/chrome/common/metrics/... File chrome/common/metrics/proto/system_profile.proto (right): https://chromiumcodereview.appspot.com/11014006/diff/1/chrome/common/metrics/... chrome/common/metrics/proto/system_profile.proto:70: optional string fingerprint = 3; How is this field going to be used?
On 2012/10/01 23:01:38, Ilya Sherman wrote: > More comments inline, but please note that if you integrate this change, it will > require server-side changes as well. If you're already uploading this data, and > merely upstreaming it here, where is it getting processed? I didn't see any > corresponding server-side changes land... This CL was supposed to be a cleanup of some changes we landed downstream a while back, where we passed the Android fingerprint as the OS version. The data is technically already reaching the servers, but it's under a different thing. It seemed odd that we were doing that so I went with adding a new field to the proto/XML. If it's logical to keep passing the fingerprint in as the OS version, it'd simplify the upstreaming of these changes...
On 2012/10/01 23:06:30, dfalcantara wrote: > On 2012/10/01 23:01:38, Ilya Sherman wrote: > > More comments inline, but please note that if you integrate this change, it > will > > require server-side changes as well. If you're already uploading this data, > and > > merely upstreaming it here, where is it getting processed? I didn't see any > > corresponding server-side changes land... > > This CL was supposed to be a cleanup of some changes we landed downstream a > while back, where we passed the Android fingerprint as the OS version. The data > is technically already reaching the servers, but it's under a different thing. > It seemed odd that we were doing that so I went with adding a new field to the > proto/XML. If it's logical to keep passing the fingerprint in as the OS > version, it'd simplify the upstreaming of these changes... It depends on what you want the fingerprint to be used for. If you want to see the fingerprint as part of the OS's name string, then it's fine to upload it as part of the name. If you'd rather have the fingerprint be separate from the OS's name string, then it's probably best to upload it separately... but keep in mind that this will require server-side changes to be able to process the new field. It would help me give you better advice on how to upload this data if I better understood how you wanted to use it.
On 2012/10/01 23:32:20, Ilya Sherman wrote: > On 2012/10/01 23:06:30, dfalcantara wrote: > > On 2012/10/01 23:01:38, Ilya Sherman wrote: > > > More comments inline, but please note that if you integrate this change, it > > will > > > require server-side changes as well. If you're already uploading this data, > > and > > > merely upstreaming it here, where is it getting processed? I didn't see any > > > corresponding server-side changes land... > > > > This CL was supposed to be a cleanup of some changes we landed downstream a > > while back, where we passed the Android fingerprint as the OS version. The > data > > is technically already reaching the servers, but it's under a different thing. > > > It seemed odd that we were doing that so I went with adding a new field to the > > proto/XML. If it's logical to keep passing the fingerprint in as the OS > > version, it'd simplify the upstreaming of these changes... > > It depends on what you want the fingerprint to be used for. If you want to see > the fingerprint as part of the OS's name string, then it's fine to upload it as > part of the name. If you'd rather have the fingerprint be separate from the > OS's name string, then it's probably best to upload it separately... but keep in > mind that this will require server-side changes to be able to process the new > field. It would help me give you better advice on how to upload this data if I > better understood how you wanted to use it. Currently we are using this fingerprint to filter our stability metrics to known/certified android builds. We have custom scripts to query and filter this data from dremel. This is why we also need this fingerprint for incremental stability data.
On 2012/10/02 00:36:07, nileshagrawal1 wrote: > On 2012/10/01 23:32:20, Ilya Sherman wrote: > > On 2012/10/01 23:06:30, dfalcantara wrote: > > > On 2012/10/01 23:01:38, Ilya Sherman wrote: > > > > More comments inline, but please note that if you integrate this change, > it > > > will > > > > require server-side changes as well. If you're already uploading this > data, > > > and > > > > merely upstreaming it here, where is it getting processed? I didn't see > any > > > > corresponding server-side changes land... > > > > > > This CL was supposed to be a cleanup of some changes we landed downstream a > > > while back, where we passed the Android fingerprint as the OS version. The > > data > > > is technically already reaching the servers, but it's under a different > thing. > > > > > It seemed odd that we were doing that so I went with adding a new field to > the > > > proto/XML. If it's logical to keep passing the fingerprint in as the OS > > > version, it'd simplify the upstreaming of these changes... > > > > It depends on what you want the fingerprint to be used for. If you want to > see > > the fingerprint as part of the OS's name string, then it's fine to upload it > as > > part of the name. If you'd rather have the fingerprint be separate from the > > OS's name string, then it's probably best to upload it separately... but keep > in > > mind that this will require server-side changes to be able to process the new > > field. It would help me give you better advice on how to upload this data if > I > > better understood how you wanted to use it. > > > Currently we are using this fingerprint to filter our stability metrics to > known/certified android builds. We have custom scripts to query and filter this > data from dremel. This is why we also need this fingerprint for incremental > stability data. If you already have scripts set up to filter based on the OS name, then at least for the XML uploads, I would continue to upload the fingerprint as part of the OS name. In fact, I wouldn't even bother upstreaming that code, since the code path is deprecated as of M21+; but if it's easier for you guys to keep track of it if it's upstreamed, that's fine too. I would strongly recommend against adding a new field to the XML uploads, as you'd then have to muck with the server, and all that work would get thrown out soon anyway. For the protocol buffer-based uploads (which is the non-deprecated upload path), you should decide whether you'd prefer to keep the fingerprint in the OS name, or split it out. Either option is fine by me. Changing the protocol buffer-based uploads doesn't actually require changes to the server itself, which is nice... though it does still require server-side changes to update the processing scripts. Having a separate field is probably a little cleaner, especially if you're currently parsing the OS string to split off the fingerprint.
On 2012/10/02 01:00:12, Ilya Sherman wrote: > On 2012/10/02 00:36:07, nileshagrawal1 wrote: > > On 2012/10/01 23:32:20, Ilya Sherman wrote: > > > On 2012/10/01 23:06:30, dfalcantara wrote: > > > > On 2012/10/01 23:01:38, Ilya Sherman wrote: > > > > > More comments inline, but please note that if you integrate this change, > > it > > > > will > > > > > require server-side changes as well. If you're already uploading this > > data, > > > > and > > > > > merely upstreaming it here, where is it getting processed? I didn't see > > any > > > > > corresponding server-side changes land... > > > > > > > > This CL was supposed to be a cleanup of some changes we landed downstream > a > > > > while back, where we passed the Android fingerprint as the OS version. > The > > > data > > > > is technically already reaching the servers, but it's under a different > > thing. > > > > > > > It seemed odd that we were doing that so I went with adding a new field to > > the > > > > proto/XML. If it's logical to keep passing the fingerprint in as the OS > > > > version, it'd simplify the upstreaming of these changes... > > > > > > It depends on what you want the fingerprint to be used for. If you want to > > see > > > the fingerprint as part of the OS's name string, then it's fine to upload it > > as > > > part of the name. If you'd rather have the fingerprint be separate from the > > > OS's name string, then it's probably best to upload it separately... but > keep > > in > > > mind that this will require server-side changes to be able to process the > new > > > field. It would help me give you better advice on how to upload this data > if > > I > > > better understood how you wanted to use it. > > > > > > Currently we are using this fingerprint to filter our stability metrics to > > known/certified android builds. We have custom scripts to query and filter > this > > data from dremel. This is why we also need this fingerprint for incremental > > stability data. > > If you already have scripts set up to filter based on the OS name, then at least > for the XML uploads, I would continue to upload the fingerprint as part of the > OS name. In fact, I wouldn't even bother upstreaming that code, since the code > path is deprecated as of M21+; but if it's easier for you guys to keep track of > it if it's upstreamed, that's fine too. I would strongly recommend against > adding a new field to the XML uploads, as you'd then have to muck with the > server, and all that work would get thrown out soon anyway. > > For the protocol buffer-based uploads (which is the non-deprecated upload path), > you should decide whether you'd prefer to keep the fingerprint in the OS name, > or split it out. Either option is fine by me. Changing the protocol > buffer-based uploads doesn't actually require changes to the server itself, > which is nice... though it does still require server-side changes to update the > processing scripts. Having a separate field is probably a little cleaner, > especially if you're currently parsing the OS string to split off the > fingerprint.
Er, this has the comment. https://chromiumcodereview.appspot.com/11014006/diff/1/chrome/browser/metrics... File chrome/browser/metrics/metrics_log.cc (right): https://chromiumcodereview.appspot.com/11014006/diff/1/chrome/browser/metrics... chrome/browser/metrics/metrics_log.cc:335: #endif On 2012/10/01 23:01:38, Ilya Sherman wrote: > I don't think it's appropriate to add the OS info as part of recording > incremental stability data. For the protocol buffer uploads, this data should > already be included as part of RecordEnvironmentProto(), which is called > immediately before RecordIncrementalStabilityElements() in the MetricsService. With nilesh's comment as context, does your comment about RecordEnvironmentProto still hold? MetricsService::StopRecording() does seem to send the proto immediately before the XML, meaning that we probably get the fingerprint twice on m18... It'd be nice to confirm this somehow, though.
https://chromiumcodereview.appspot.com/11014006/diff/1/chrome/browser/metrics... File chrome/browser/metrics/metrics_log.cc (right): https://chromiumcodereview.appspot.com/11014006/diff/1/chrome/browser/metrics... chrome/browser/metrics/metrics_log.cc:335: #endif On 2012/10/02 18:24:24, dfalcantara wrote: > On 2012/10/01 23:01:38, Ilya Sherman wrote: > > I don't think it's appropriate to add the OS info as part of recording > > incremental stability data. For the protocol buffer uploads, this data should > > already be included as part of RecordEnvironmentProto(), which is called > > immediately before RecordIncrementalStabilityElements() in the MetricsService. > > With nilesh's comment as context, does your comment about RecordEnvironmentProto > still hold? MetricsService::StopRecording() does seem to send the proto > immediately before the XML, meaning that we probably get the fingerprint twice > on m18... It'd be nice to confirm this somehow, though. TL;DR: You can ignore my previous comment w.r.t. the XML uploads, as long as the changes only affect Android. To clarify: There are no protocol buffer uploads in m18; just XML ones. In m21+, there are both XML and protocol buffer uploads, which upload more or less the same data. The XML uploading mechanism is deprecated, and will soon be removed. Adding a new field to the XML uploads requires server-side changes for parsing the XML, which just isn't worth at this point. So, there are two questions: (1) What, if anything, should we upstream for the XML uploads? (2) How should we add the fingerprints to the protocol buffer uploads? For (1), I don't have a strong preference for whether or not we upstream some Android-only code; but we definitely shouldn't add any new XML fields (tags). If you want to upstream the changes currently in Android, that's fine, but in that case please leave the fingerprint as part of the OS name.
https://chromiumcodereview.appspot.com/11014006/diff/1/chrome/browser/metrics... File chrome/browser/metrics/metrics_log.cc (right): https://chromiumcodereview.appspot.com/11014006/diff/1/chrome/browser/metrics... chrome/browser/metrics/metrics_log.cc:335: #endif > TL;DR: You can ignore my previous comment w.r.t. the XML uploads, as long as the > changes only affect Android. > > To clarify: There are no protocol buffer uploads in m18; just XML ones. In > m21+, there are both XML and protocol buffer uploads, which upload more or less > the same data. The XML uploading mechanism is deprecated, and will soon be > removed. Adding a new field to the XML uploads requires server-side changes for > parsing the XML, which just isn't worth at this point. > > So, there are two questions: (1) What, if anything, should we upstream for the > XML uploads? (2) How should we add the fingerprints to the protocol buffer > uploads? > > For (1), I don't have a strong preference for whether or not we upstream some > Android-only code; but we definitely shouldn't add any new XML fields (tags). > If you want to upstream the changes currently in Android, that's fine, but in > that case please leave the fingerprint as part of the OS name. If that's the case then we really shouldn't be forking it on the Android side; I'm removing the diff from our downstream master branch. The diff in the m18 release branch isn't being upstreamed and will continue overriding the OS version in the XML. For (2) I'd still lean towards adding the fingerprint as a separate field in the protobuf definition. Apparently the only thing we need to change after adding the new field is a dremel query or two.
Removed changes to the XML structure and went with the protobuf changes. Also filed a bug to change our local scripts to pull the right info for new versions on the master branch. https://chromiumcodereview.appspot.com/11014006/diff/1/chrome/browser/metrics... File chrome/browser/metrics/metrics_log.cc (right): https://chromiumcodereview.appspot.com/11014006/diff/1/chrome/browser/metrics... chrome/browser/metrics/metrics_log.cc:901: // Write the XML version. On 2012/10/01 23:01:38, Ilya Sherman wrote: > As I mentioned above, I don't think you should move this code. However, if you > do, you should document why this method only writes the XML version, and > probably name it accordingly. Undid the change. We'll go with the protobuf on master. https://chromiumcodereview.appspot.com/11014006/diff/1/chrome/browser/metrics... chrome/browser/metrics/metrics_log.cc:907: base::android::BuildInfo::GetInstance()->android_build_fp()); On 2012/10/01 23:01:38, Ilya Sherman wrote: > I don't recommend trying to add this field to the XML-based uploads, as that > pipeline is deprecated. The only reason to add this field to the XML uploads is > if you need it like yesterday for M18-based Android builds... but then there's > not much point to upstreaming the change to the main chrome/ repository. Removed.
Thanks, metrics changes LGTM
Cool beans, thanks! Adding willchan for base/ changes.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/11014006/16001
Change committed as 159957 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
