DeviceInfo into GenericSets

DeviceInfo into GenericSets

This is part of the effort to move some of our diagnostics into
GenericSet diagnostics rather than being their own type when appropriate.

This CL converts the DeviceInfo diagnostic class into a few Generic Sets:
one each for the fields of OS Name, OS Version, Architecture, Available
Memory, GPU Info, and Chrome version.

The same removal process has been done for MergedDeviceInfo, since that
is not necessary anymore.

Among other things, this CL:
* Removes the DeviceInfo Diagnostic class from both Python and JS
  implementations.

* Removes DeviceInfo and MergedDeviceInfo from all_diagnostics.

* Changes diagnostic instantiation in add_device_info to use
  GenericSets and HistogramSet's AddSharedDiagnostic, replacing
  DeviceInfo and using Vinn, respectively.

* Removes DeviceInfo from being one of the SparseDiagnostics
  type.

* Removes UI implementations in Tracing for DeviceInfo and
  MergedDeviceInfo.

* Remove DeviceInfo from add_histogram and respective tests.

* Removes documentation regarding DeviceInfo.

What has not been addressed:

* We are still missing SparseDiagnostics picking diagnostics by name
  rather than by type. To be done in a separate bug.

* Documentation on how the new GenericSet diagnostics work. Also
  registered in a separate bug.

BUG=catapult:#3738,catapult:#3507

Review-Url: https://codereview.chromium.org/2988823002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/21507ebaa1459e80544a73e0939d0a665b9d90f6

[phsilva, 2017-07-25 21:15:39]

phsilva@google.com changed reviewers:
+ benjhayden@chromium.org, eakuefner@chromium.org, simonhatch@chromium.org

[phsilva, 2017-07-25 21:15:40]

Hi all,

Here's the CL for making DeviceInfo into GenericSet diagnostics. I've added an
overall description of what we are removing, and added a few comments in the
Review of parts of the code I wanted some more guidance in. Let me know what you
think.

https://codereview.chromium.org/2988823002/diff/1/tracing/tracing/value/add_d...
File tracing/tracing/value/add_device_info.py (right):

https://codereview.chromium.org/2988823002/diff/1/tracing/tracing/value/add_d...
tracing/tracing/value/add_device_info.py:8: def AddDeviceInfo(histogram_set,
chrome_version, os_name, os_version,
Currently, this function is not being used anywhere, as far as I could tell. I
figured I would update it rather than entirely deleting it for now. As part of
the effort to populate DeviceInfo in the near future, we may or may not need it,
so I suggest we push the decision to then and keep as an available resource for
now.

https://codereview.chromium.org/2988823002/diff/1/tracing/tracing/value/add_d...
tracing/tracing/value/add_device_info.py:31: for device_info_name,
device_info_value in device_info.iteritems():
This part of the code adds GenericSet diagnostics for all of the above, even if
they turn out to be "falsy" -- None, or an empty list. Do we want to ignore the
ones that are going to be empty, in order to avoid volume, or is it not
significant enough.

[benjhayden, 2017-07-26 05:21:18]

The CL description mentions owners and bug components. Cruft?

https://codereview.chromium.org/2988823002/diff/1/tracing/tracing/value/add_d...
File tracing/tracing/value/add_device_info.py (right):

https://codereview.chromium.org/2988823002/diff/1/tracing/tracing/value/add_d...
tracing/tracing/value/add_device_info.py:31: for device_info_name,
device_info_value in device_info.iteritems():
On 2017/07/25 at 21:15:39, phsilva wrote:
> This part of the code adds GenericSet diagnostics for all of the above, even
if they turn out to be "falsy" -- None, or an empty list. Do we want to ignore
the ones that are going to be empty, in order to avoid volume, or is it not
significant enough.

I think you can say 'if device_info_value:' here in order to avoid storing empty
GenericSets.
I think that callers are going to need to be prepared for histograms to lack
these diagnostics regardless of what you do here because not all histograms will
necessarily pass through here.
Does that sound right?

[benjhayden, 2017-07-26 05:21:21]

The CL description mentions owners and bug components. Cruft?

[benjhayden, 2017-07-26 05:24:14]

Other than those nits, lgtm, thank you so much for taking care of this!

https://codereview.chromium.org/2988823002/diff/20001/docs/how-to-write-metri...
File docs/how-to-write-metrics.md (left):

https://codereview.chromium.org/2988823002/diff/20001/docs/how-to-write-metri...
docs/how-to-write-metrics.md:198:
![](/docs/images/how-to-write-metrics-device.png)
Can you git rm this image file as well?

[shatch, 2017-07-26 12:44:06]

dashboard lgtm

[phsilva, 2017-07-26 15:11:06]

Description was changed from

==========
DeviceInfo into GenericSets

This is part of the effort to move some of our diagnostics into
GenericSet diagnostics rather than being their own type when appropriate.

This CL converts the DeviceInfo diagnostic class into a few Generic Sets:
one each for the fields of OS Name, OS Version, Architecture, Available
Memory, GPU Info, and Chrome version.

The same removal process has been done for MergedDeviceInfo, since that
is not necessary anymore.

Among other things, this CL:
* Removes the DeviceInfo Diagnostic class from both Python and JS
  implementations.

* Sets the reserved names for owners and bug components to use
  GenericSet instead of DeviceInfo.

* Removes DeviceInfo and MergedDeviceInfo from all_diagnostics.

* Changes diagnostic instantiation in add_device_info to use
  GenericSets and HistogramSet's AddSharedDiagnostic, replacing
  DeviceInfo and using Vinn, respectively.

* Removes DeviceInfo from being one of the SparseDiagnostics
  type.

* Removes UI implementations in Tracing for DeviceInfo and
  MergedDeviceInfo.

* Remove DeviceInfo from add_histogram and respective tests.

* Removes documentation regarding DeviceInfo.

What has not been addressed:

* We are still missing SparseDiagnostics picking diagnostics by name
  rather than by type. To be done in a separate bug.

* Documentation on how the new GenericSet diagnostics work. Also
  registered in a separate bug.

BUG=catapult:#3738,catapult:#3507
==========

to

==========
DeviceInfo into GenericSets

This is part of the effort to move some of our diagnostics into
GenericSet diagnostics rather than being their own type when appropriate.

This CL converts the DeviceInfo diagnostic class into a few Generic Sets:
one each for the fields of OS Name, OS Version, Architecture, Available
Memory, GPU Info, and Chrome version.

The same removal process has been done for MergedDeviceInfo, since that
is not necessary anymore.

Among other things, this CL:
* Removes the DeviceInfo Diagnostic class from both Python and JS
  implementations.

* Removes DeviceInfo and MergedDeviceInfo from all_diagnostics.

* Changes diagnostic instantiation in add_device_info to use
  GenericSets and HistogramSet's AddSharedDiagnostic, replacing
  DeviceInfo and using Vinn, respectively.

* Removes DeviceInfo from being one of the SparseDiagnostics
  type.

* Removes UI implementations in Tracing for DeviceInfo and
  MergedDeviceInfo.

* Remove DeviceInfo from add_histogram and respective tests.

* Removes documentation regarding DeviceInfo.

What has not been addressed:

* We are still missing SparseDiagnostics picking diagnostics by name
  rather than by type. To be done in a separate bug.

* Documentation on how the new GenericSet diagnostics work. Also
  registered in a separate bug.

BUG=catapult:#3738,catapult:#3507
==========

[phsilva, 2017-07-26 15:12:50]

On 2017/07/26 at 05:21:21, benjhayden wrote:
> The CL description mentions owners and bug components. Cruft?

Thanks for pointing that out, I thought I got rid of all of those! Sorry for the
oversight!

[eakuefner, 2017-07-26 22:48:36]

lgtm

[phsilva, 2017-07-26 22:52:32]

The CQ bit was checked by phsilva@google.com

[phsilva, 2017-07-26 22:52:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
benjhayden@chromium.org, simonhatch@chromium.org
Link to the patchset: https://codereview.chromium.org/2988823002/#ps40001
(title: "Remove how-to-write-metrics-device image")

[commit-bot: I haz the power, 2017-07-26 22:52:44]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-26 22:55:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-26 22:55:57]

Try jobs failed on following builders:
  Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)

[phsilva, 2017-07-27 20:05:00]

The CQ bit was checked by phsilva@google.com

[commit-bot: I haz the power, 2017-07-27 20:05:12]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-27 20:09:06]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1501185900985260,
"parent_rev": "cc7953a310bec01d56f52be2357db78af412d957", "commit_rev":
"21507ebaa1459e80544a73e0939d0a665b9d90f6"}

[commit-bot: I haz the power, 2017-07-27 20:09:09]

Description was changed from

==========
DeviceInfo into GenericSets

This is part of the effort to move some of our diagnostics into
GenericSet diagnostics rather than being their own type when appropriate.

This CL converts the DeviceInfo diagnostic class into a few Generic Sets:
one each for the fields of OS Name, OS Version, Architecture, Available
Memory, GPU Info, and Chrome version.

The same removal process has been done for MergedDeviceInfo, since that
is not necessary anymore.

Among other things, this CL:
* Removes the DeviceInfo Diagnostic class from both Python and JS
  implementations.

* Removes DeviceInfo and MergedDeviceInfo from all_diagnostics.

* Changes diagnostic instantiation in add_device_info to use
  GenericSets and HistogramSet's AddSharedDiagnostic, replacing
  DeviceInfo and using Vinn, respectively.

* Removes DeviceInfo from being one of the SparseDiagnostics
  type.

* Removes UI implementations in Tracing for DeviceInfo and
  MergedDeviceInfo.

* Remove DeviceInfo from add_histogram and respective tests.

* Removes documentation regarding DeviceInfo.

What has not been addressed:

* We are still missing SparseDiagnostics picking diagnostics by name
  rather than by type. To be done in a separate bug.

* Documentation on how the new GenericSet diagnostics work. Also
  registered in a separate bug.

BUG=catapult:#3738,catapult:#3507
==========

to

==========
DeviceInfo into GenericSets

This is part of the effort to move some of our diagnostics into
GenericSet diagnostics rather than being their own type when appropriate.

This CL converts the DeviceInfo diagnostic class into a few Generic Sets:
one each for the fields of OS Name, OS Version, Architecture, Available
Memory, GPU Info, and Chrome version.

The same removal process has been done for MergedDeviceInfo, since that
is not necessary anymore.

Among other things, this CL:
* Removes the DeviceInfo Diagnostic class from both Python and JS
  implementations.

* Removes DeviceInfo and MergedDeviceInfo from all_diagnostics.

* Changes diagnostic instantiation in add_device_info to use
  GenericSets and HistogramSet's AddSharedDiagnostic, replacing
  DeviceInfo and using Vinn, respectively.

* Removes DeviceInfo from being one of the SparseDiagnostics
  type.

* Removes UI implementations in Tracing for DeviceInfo and
  MergedDeviceInfo.

* Remove DeviceInfo from add_histogram and respective tests.

* Removes documentation regarding DeviceInfo.

What has not been addressed:

* We are still missing SparseDiagnostics picking diagnostics by name
  rather than by type. To be done in a separate bug.

* Documentation on how the new GenericSet diagnostics work. Also
  registered in a separate bug.

BUG=catapult:#3738,catapult:#3507

Review-Url: https://codereview.chromium.org/2988823002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-07-27 20:09:10]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...