First draft of the metrics doc.

First draft of the metrics doc.

[sosa, 2010-05-06 17:57:05]

lgtm, nice

[kmixter1, 2010-05-06 20:01:21]

LGTM very nice, just some longer term comments and a question you might address.

http://codereview.chromium.org/1989004/diff/1/2
File src/platform/metrics/README (right):

http://codereview.chromium.org/1989004/diff/1/2#newcode31
src/platform/metrics/README:31: <metrics_library.h> header file. The file is
installed in
I also think we should not put this into /usr/include directly and instead into
a subdir, but it doesn't have to happen now.

http://codereview.chromium.org/1989004/diff/1/2#newcode37
src/platform/metrics/README:37: bool MetricsLibrary::SendToChrome(const
std::string& name, int sample,
If you want this to be useful as a dynamically loaded .so I think you'll want to
avoid C++ function APIs.

http://codereview.chromium.org/1989004/diff/1/2#newcode58
src/platform/metrics/README:58: Use FocusArea.MetricName. For example:
Do you want to explain that Focus Area should be a tracker area?

http://codereview.chromium.org/1989004/diff/1/2#newcode74
src/platform/metrics/README:74: under the Histograms tab).
Does this have to happen before the release or can it happen after and still get
the data from the first day?

[petkov, 2010-05-11 00:28:02]

Thanks for the review, I think I've addressed the comments.

http://codereview.chromium.org/1989004/diff/1/2
File src/platform/metrics/README (right):

http://codereview.chromium.org/1989004/diff/1/2#newcode31
src/platform/metrics/README:31: <metrics_library.h> header file. The file is
installed in
On 2010/05/06 20:01:22, kmixter1 wrote:
> I also think we should not put this into /usr/include directly and instead
into
> a subdir, but it doesn't have to happen now.

Yeah, probably... Although, at least name collision is pretty unlikely given
that we use underscore-based naming scheme rather than dash-based. Do we do
separate subdir for each package?

Anyway, not a doc issue really.

http://codereview.chromium.org/1989004/diff/1/2#newcode37
src/platform/metrics/README:37: bool MetricsLibrary::SendToChrome(const
std::string& name, int sample,
On 2010/05/06 20:01:22, kmixter1 wrote:
> If you want this to be useful as a dynamically loaded .so I think you'll want
to
> avoid C++ function APIs.

Good point -- one would have hard time using the .so through dlopen/dlsym. But
we can address this once it becomes an issue -- for now, all clients just link
against the .so explicitly.

http://codereview.chromium.org/1989004/diff/1/2#newcode58
src/platform/metrics/README:58: Use FocusArea.MetricName. For example:
On 2010/05/06 20:01:22, kmixter1 wrote:
> Do you want to explain that Focus Area should be a tracker area?

Done.

http://codereview.chromium.org/1989004/diff/1/2#newcode74
src/platform/metrics/README:74: under the Histograms tab).
On 2010/05/06 20:01:22, kmixter1 wrote:
> Does this have to happen before the release or can it happen after and still
get
> the data from the first day?

I've updated the doc with some detailed explanation.

[kmixter1, 2010-05-12 03:32:54]

LGTM

On Mon, May 10, 2010 at 5:28 PM,  <petkov@chromium.org> wrote:
> Thanks for the review, I think I've addressed the comments.
>
>
>
> http://codereview.chromium.org/1989004/diff/1/2
> File src/platform/metrics/README (right):
>
> http://codereview.chromium.org/1989004/diff/1/2#newcode31
> src/platform/metrics/README:31: <metrics_library.h> header file. The
> file is installed in
> On 2010/05/06 20:01:22, kmixter1 wrote:
>>
>> I also think we should not put this into /usr/include directly and
>
> instead into
>>
>> a subdir, but it doesn't have to happen now.
>
> Yeah, probably... Although, at least name collision is pretty unlikely
> given that we use underscore-based naming scheme rather than dash-based.
> Do we do separate subdir for each package?
>
> Anyway, not a doc issue really.
>
> http://codereview.chromium.org/1989004/diff/1/2#newcode37
> src/platform/metrics/README:37: bool MetricsLibrary::SendToChrome(const
> std::string& name, int sample,
> On 2010/05/06 20:01:22, kmixter1 wrote:
>>
>> If you want this to be useful as a dynamically loaded .so I think
>
> you'll want to
>>
>> avoid C++ function APIs.
>
> Good point -- one would have hard time using the .so through
> dlopen/dlsym. But we can address this once it becomes an issue -- for
> now, all clients just link against the .so explicitly.
>
> http://codereview.chromium.org/1989004/diff/1/2#newcode58
> src/platform/metrics/README:58: Use FocusArea.MetricName. For example:
> On 2010/05/06 20:01:22, kmixter1 wrote:
>>
>> Do you want to explain that Focus Area should be a tracker area?
>
> Done.
>
> http://codereview.chromium.org/1989004/diff/1/2#newcode74
> src/platform/metrics/README:74: under the Histograms tab).
> On 2010/05/06 20:01:22, kmixter1 wrote:
>>
>> Does this have to happen before the release or can it happen after and
>
> still get
>>
>> the data from the first day?
>
> I've updated the doc with some detailed explanation.
>
> http://codereview.chromium.org/1989004/show
>