1276195 - Add scalar measurements API & storage in Telemetry C++ core

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Georg Fritzsche [:gfritzsche]]

We will manage the scalar data in the Telemetry C++ core, starting out with support for int and string scalars.
We will have APIs for C++ and JS (nsITelemetry) that allow recording and serialization.

Required API for both keyed & plain scalars (to cover the initial needs):
* set value (int, string)
* add to value (int)
* serialize all scalars in dataset D to JS (only exposed on nsITelemetry)

Other needs:
* implement in TelemetryScalar.h/.cpp, mimicking the locking approach for TelemetryHistogram from bug 1258183
* variant storage to support the different types
* lazy |name -> enum| cache to speed up lookups

Comment 1

[Georg Fritzsche [:gfritzsche]]

Given the needs for bug 1252625, we can also start out with just the non-keyed scalars now and start supporting keyed scalars in a follow-up bug.

Comment 2

[Alessio Placitelli [:Dexter]]

Attachment: WIP patch

Comment 3

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8758778 [details] [diff] [review]
WIP patch

Review of attachment 8758778 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/nsITelemetry.idl
@@ +385,5 @@
> +
> +  /**
> +   * TBD.
> +   */
> +  void scalarAdd(in ACString name, in unsigned long value);

Can we use a "variant" type for the |value| parameter?
Then we can use the same function for different types.

Also, we need to define a C++ API in Telemetry.h.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Note that for the first cut of this, we can ignore recording in content processes.
After bug 1218576 is settled, we can mimic that approach for scalar measurements in a follow-up.
For now, ignoring / not recording scalars in the content process is fine.

Comment 5

[Alessio Placitelli [:Dexter]]

Attachment: Scalar_API.diff

Comment 6

[Alessio Placitelli [:Dexter]]

Comment on attachment 8759642 [details] [diff] [review]
Scalar_API.diff

We're adding a new type of Telemetry measurement, the Scalar type (context in [1]). This will allow us to submit scalar data (for e.g. counts, feature checks, chipset names, ...) without overloading the Histogram type.

@MattN, this patch illustrates the API we'd like to implement. What do you think about it?

[1] - https://docs.google.com/document/d/13mcY17RzpE3BsB4CgR6DidytWzl4tMpAEmrvf8fOSJ8/edit#

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8759642 [details] [diff] [review]
Scalar_API.diff

Review of attachment 8759642 [details] [diff] [review]:
-----------------------------------------------------------------

I gave feedback in the document.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Could you still give feedback on the more detailed proposed API here?

Comment 9

[Georg Fritzsche [:gfritzsche]]

One alternative style i could think of:
> Services.telemetry.scalars.add("browser.usage.tabs.open_events", 1)
> Services.telemetry.scalars.set("environment.gfx.device_id", "0x5fad")

That would mean introducing a separate `nsITelemetryScalar`, which would keep `nsITelemetry` from growing even more and avoids the prefixing (`telemetry.scalarAdd()` etc.).
We could later move histograms to a similar interface, accessible as `Services.telemetry.histograms`.

Comment 10

[Alessio Placitelli [:Dexter]]

Attachment: WIP patch

This is a super ugly WIP patch with a minimal XPCSHELL test example.

Comment 11

[Alessio Placitelli [:Dexter]]

Attachment: WIP patch

Comment 12

[Alessio Placitelli [:Dexter]]

Attachment: WIP patch

There are some TODOs around in the patch, with open questions. Among them:

- How should we deal with empty scalars (i.e. scalars that were set but are cleared after a subsession split)? Should we report them as empty in the snapshot or simply skip them? With the exception of flag histograms in the future.
- How should we deal with the way we store canRecordBase/Extended? Now that we have scalars, should the state still be stored in TelemetryHistograms.cpp?
- Should we free the storage memory on shutdown? It looks like we don't do that for histograms (probably for speed reasons?).

Also, this patch must be rebased given the TSan changes that are landing.

Comment 13

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #12)
> - How should we deal with empty scalars (i.e. scalars that were set but are
> cleared after a subsession split)? Should we report them as empty in the
> snapshot or simply skip them? With the exception of flag histograms in the
> future.

We should skip them (or internally, probably just clear the storage) - empty scalars don't really make sense.

> - How should we deal with the way we store canRecordBase/Extended? Now that
> we have scalars, should the state still be stored in TelemetryHistograms.cpp?

This overlaps with bug 1271991.
We don't want to have Telemetry{Histogram,Scalar} calling up into Telemetry.cpp, so the idea is:
* track the recording state in Telemetry.cpp to avoid excessive locking for calls to GetCanRecordBase() etc.
* also track the recording state in Telemetry{Histogram,Scalar} to avoid calling upwards in the hierarchy
* on each change to recording state in Telemetry.cpp, propagate it down to Telemetry{Histogram,Scalar}

> - Should we free the storage memory on shutdown? It looks like we don't do
> that for histograms (probably for speed reasons?).

This is mostly a question on whether we need to do this.
I assume we have to because of leak detection etc.

Comment 14

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8761529 [details] [diff] [review]
WIP patch

Review of attachment 8761529 [details] [diff] [review]:
-----------------------------------------------------------------

A more general note:
I notice that we should enforce, document and test a length limit on strings.
We should rather err on the low end here (say 50?), we could always bump it later if needed.
Please flag bsmedberg if he has an opinion on values from the data collection perspective.

I skipped TelemetryScalar in this pass.

::: toolkit/components/telemetry/Telemetry.cpp
@@ +1939,5 @@
>                          XRE_IsParentProcess() || XRE_IsContentProcess());
>  
> +  TelemetryScalar::InitializeGlobalState(
> +                     XRE_IsParentProcess() || XRE_IsContentProcess(),
> +                     XRE_IsParentProcess() || XRE_IsContentProcess());

We are not supporting recording scalars in content processes yet.

@@ +2346,5 @@
> +{
> +  // Unpack the aVal to nsIVariant. This uses the JS context.
> +  nsCOMPtr<nsIVariant> unpackedVal;
> +  nsresult rv =
> +    nsContentUtils::XPConnect()->JSToVariant(aCx, aVal,  getter_AddRefs(unpackedVal));

I think that belongs into TelemetryScalar - as-is this leaks argument checking logic etc. into Telemetry.cpp

@@ +2942,5 @@
> +void
> +ScalarAdd(const nsACString &aName, uint32_t aVal)
> +{
> +  nsCOMPtr<nsIWritableVariant> var(new nsVariant());
> +  nsresult rv = var->SetAsUint32(aVal);

Why do we need to do that? This is a redundant conversion.
You can overload `Telemetry::ScalarAdd()` as needed.
Ditto below.

::: toolkit/components/telemetry/Telemetry.h
@@ +313,5 @@
> + *
> + * @param aName The scalar name.
> + * @param aValue The unsigned value to add to the scalar.
> + */
> +void ScalarAdd(const nsCString& aName, uint32_t aValue);

We don't need to support calling by name for the C++ API, only by enum id.

::: toolkit/components/telemetry/TelemetryCommon.h
@@ +42,5 @@
>    return true;
>  }
>  
> +
> +namespace TelemetryCommon {

Any reason to not use nested namespaces?
While we are here, let's move the other types here also into the namespace.

::: toolkit/components/telemetry/TelemetryScalar.cpp
@@ +58,5 @@
> +/**
> + * The base scalar object, that servers as a common ancestor for storage
> + * purposes.
> + */
> +class BaseScalar

`ScalarBase`?

@@ +62,5 @@
> +class BaseScalar
> +{
> +public:
> +  virtual ~BaseScalar() {};
> +  virtual nsresult SetValue(nsIVariant *aValue) = 0;

Why not `const nsIVariant&`?

@@ +65,5 @@
> +  virtual ~BaseScalar() {};
> +  virtual nsresult SetValue(nsIVariant *aValue) = 0;
> +  virtual nsresult AddValue(nsIVariant *aValue) { return NS_ERROR_NOT_AVAILABLE; }
> +  virtual nsresult SetMaximum(nsIVariant *aValue) { return NS_ERROR_NOT_AVAILABLE; }
> +  virtual nsresult GetValue(nsIVariant **aResult) = 0;

This should probably use one of our smart pointers here.

@@ +85,5 @@
> +  // classes really should be able to.
> +  ScalarSet() : mMutex("ScalarSet(BaseScalar)") {};
> +  T mStorage;
> +  // Protects all data fields.
> +  mutable OffTheBooksMutex mMutex;

Did you follow bug 1258183 et al?
We want the locking on the public interface level of this module instead of fine-grained internal locking.

With the mutex out of `ScalarSet`, do we have other common logic we could put into `ScalarSet`?
Otherwise i doubt `ScalarSet` is still useful to have (the only shared thing being the `T mStorage`.

@@ +101,5 @@
> +{
> +public:
> +  ~ScalarUnsigned() {};
> +
> +  nsresult SetValue(nsIVariant *aValue) override

We should keep class declaration and definition separate.
Let's default to `final` instead of `override`.

@@ +105,5 @@
> +  nsresult SetValue(nsIVariant *aValue) override
> +  {
> +    OffTheBooksMutexAutoLock locker(mMutex);
> +    return aValue->GetAsUint32(&mStorage);
> +  };

You don't need semicolons after method definitions.

@@ +151,5 @@
> +    mStorage = 0;
> +  };
> +
> +  static BaseScalar* GetInstance() {
> +    return new ScalarUnsigned();

What do we get from those `T::GetInstance()` functions?
Why not just use `new T()` below?

@@ +157,5 @@
> +
> +private:
> +  // Make the ctor private to make this object only instantiable via its ::GetInstance()
> +  // method.
> +  ScalarUnsigned() { Clear(); };

Why Clear()?
Why not just: ScalarUnsigned() : mStorage() {}

@@ +226,5 @@
> +bool gCanRecordExtended;
> +
> +const uint32_t gStorageSize = static_cast<uint32_t>(mozilla::Telemetry::ScalarID::ScalarCount);
> +ScalarMapType gScalarNameIDMap(gStorageSize);
> +std::vector<BaseScalar*> gScalarStorage(gStorageSize);

We usually don't use standard library containers, see:
https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code#C_and_Mozilla_standard_libraries

Tracking this with plain pointers means you have to manually track what should be deleted or not.
Please use smart pointers or pointer containers, depending on what we prefer or have available in this kind of context.

Why does this use a vector-like structure?
Why not a map/hashmap of (id -> instance)?
Any arguments either way?

@@ +402,5 @@
> +  gScalarNameIDMap.Clear();
> +  gInitDone = false;
> +
> +  // TODO: Should we free the memory from the scalar storage? I'm not sure we're doing
> +  // that for histograms. I think we're intentionally leaking them.

That will set off leak detection and that we need to clear things out.
The histogram storage from histogram.cc is not leaked and just a bit hidden via the StatisticsRecorder.

@@ +629,5 @@
> +    // Skip invalid scalars.
> +    if (!scalar) {
> +      continue;
> +    }
> +    scalar->Clear();

Could/should we simply clear the storage contents?

::: toolkit/components/telemetry/nsITelemetry.idl
@@ +11,5 @@
>  {
>    void complete();
>  };
>  
> +[scriptable, uuid(a0320f2e-451e-452f-b069-a93be00129da)]

We should not rev them anymore, see:
https://groups.google.com/d/msg/mozilla.dev.platform/HE1_qZhPj1I/rElS7KjXAQAJ

@@ +388,5 @@
> +   *
> +   * @param aName The scalar name.
> +   * @param aValue The numeric value to add to the scalar. Declared as a jsval
> +   *        to support different numeric types. Only unsigned integers supported
> +   *        at this time.

I don't think we need the justification in the comments here (i.e. "Declared as a ...").
Let's drop the "at this time". If we start support other things we'll update the comment.

Ditto for the ones below.

@@ +416,5 @@
> +  void scalarSetMaximum(in ACString aName, in jsval aValue);
> +
> +  /**
> +   * Serializes the scalars from the given dataset to a json-style object and resets them.
> +   * The returned structure looks like {"group1.probe":1,"group1.other_probe":false,...}.

We don't need to be tight on comment length/lines.
Please add whitespace after seperators and put the example on a new line.

::: toolkit/components/telemetry/tests/unit/test_TelemetryScalars.js
@@ +3,5 @@
> +*/
> +
> +add_task(function* test_serializationFormat() {
> +  const UINT_SCALAR = "telemetry.test.unsigned_int_kind";
> +  const STRING_SCALAR = "telemetry.test.string_kind";

We use them below too. Just put the constants global in this file?

@@ +19,5 @@
> +  // Check that they are serialized to the correct format.
> +  Assert.equal(typeof(scalars[UINT_SCALAR]), "number",
> +               UINT_SCALAR + " must be serialized to the correct format.");
> +  Assert.ok(Number.isInteger(scalars[UINT_SCALAR]),
> +               UINT_SCALAR + " must be a finite integer.");

Also test for the correct values for the probes?

@@ +24,5 @@
> +  Assert.equal(typeof(scalars[STRING_SCALAR]), "string",
> +               STRING_SCALAR + " must be serialized to the correct format.");
> +});
> +
> +add_task(function* test_expiredScalar() {

Are you also testing an unexpired scalar with an explicit expiry (e.g. "200") somewhere?

@@ +112,5 @@
> +  Telemetry.scalarSet(STRING_SCALAR, expected);
> +  checkExpectedString(expected);
> +
> +  // We have some unsupported operations for strings.
> +  Assert.throws(() => Telemetry.scalarAdd(STRING_SCALAR, 1),

Also cover the unsupported operations with string arguments.

@@ +120,5 @@
> +                /NS_ERROR_NOT_AVAILABLE/,
> +                "Using an unsupported operation must throw.");
> +
> +  // What happens if we try to set a value of a different type?
> +  Telemetry.scalarSet(STRING_SCALAR, 1);

So passing arguments of the wrong type throws for uint probes, but not for string ones?
Lets be consistent and document the behavior clearly in the IDL comments.

@@ +133,5 @@
> +    const dataset = optIn ? Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN
> +                          : Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTOUT;
> +    const scalarName = optIn ? OPTIN_SCALAR : OPTOUT_SCALAR;
> +    const scalars = Telemetry.snapshotScalars(dataset, false);
> +    // Some scalars are not persisted if they are not set.

What does that mean? The scalars we are supporting now should have the same behavior.

@@ +147,5 @@
> +  Telemetry.canRecordExtended = false;
> +  Telemetry.clearScalars();
> +
> +  // Make sure we have a baseline value.
> +  checkScalar(false /* opt-out */, 0);

I find this a little hard to follow with the hidden name choosing.
Can we instead do something more explicitly like:
checkValue(probeName, ...)
checkNotSerialized(probename)

@@ +189,5 @@
> +  Assert.equal(scalars[STRING_SCALAR], "some value",
> +               STRING_SCALAR + " must contain the expected value.");
> +
> +  // Get a new snapshot and reset the subsession again. Since no new value
> +  // was set, the scalars should be empty.

They should not be present at all.

Comment 15

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #14)
> I skipped TelemetryScalar in this pass.

Never mind that part, i didn't.

Comment 16

[Alessio Placitelli [:Dexter]]

Attachment: bug1276195.patch

Comment 17

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #14)
> Comment on attachment 8761529 [details] [diff] [review]
> WIP patch
> 
> Review of attachment 8761529 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A more general note:
> I notice that we should enforce, document and test a length limit on strings.
> We should rather err on the low end here (say 50?), we could always bump it
> later if needed.
> Please flag bsmedberg if he has an opinion on values from the data
> collection perspective.

@Benjamin, do you have an opinion on this?

Comment 18

[Alessio Placitelli [:Dexter]]

I'm clearing the ni? here, moving it to f? on bug 1276190.

Comment 19

[Nathan Froyd [:froydnj]]

Comment on attachment 8762007 [details] [diff] [review]
bug1276195.patch

Review of attachment 8762007 [details] [diff] [review]:
-----------------------------------------------------------------

I have some style and refactoring comments.  I don't understand the motivation or the intended purpose well enough to comment on the suitability of this; I trust that Georg will take care of that part.

I did not look through the tests either, for much the same reasons.

I think the memory reporting parts are OK.

::: toolkit/components/telemetry/Telemetry.cpp
@@ +1092,5 @@
>    JS::Rooted<JSObject*> statsObj(cx, JS_NewPlainObject(cx));
>    if (!statsObj)
>      return false;
>  
> +  Telemetry::Common::AutoHashtable<SlowSQLEntryType> &sqlMap =

Gecko style is to associate '&' and '*' with the type, so this ought to be:

  ...AutoHashtable<SlowSQLEntryType>& sqlMap = ...

I realize that the declaration of this function uses non-Gecko style, but I see Gecko style used in other places throughout this file, so we might as well try to make new code use proper style.

At the very least, please make TelemetryScalar.{h,cpp} use Gecko style for this.

::: toolkit/components/telemetry/Telemetry.h
@@ +343,5 @@
> +/**
> + * Sets the scalar to the maximum of the current and the passed value.
> + *
> + * @param aId The scalar enum id.
> + * @param aValue The unsigned value to set the scalar to.

This is not quite a correct description of this parameter.

::: toolkit/components/telemetry/TelemetryScalar.cpp
@@ +160,5 @@
> +  nsresult rv = outVar->SetAsUint32(mStorage);
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +  aResult = outVar;

aResult = outVar.forget(), please.

@@ +238,5 @@
> +  nsresult rv = outVar->SetAsAString(mStorage);
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +  aResult = outVar;

aResult = outVar.forget(), please.

@@ +250,5 @@
> +  n+= mStorage.SizeOfExcludingThisIfUnshared(aMallocSizeOf);
> +  return n;
> +}
> +
> +typedef nsClassHashtable<nsUint32HashKey, ScalarBase> ScalarStorageMapType;

Even if you have to write a little more code, I think it would be preferable to make this:

nsClassHashtable<ScalarIDHashKey, ScalarBase>

so that you can avoid some casting to uint32_t in various places.

@@ +466,5 @@
> +
> +void
> +DeInitializeGlobalState()
> +{
> +  StaticMutexAutoLock locker(gTelemetryScalarsMutex);

Earlier you state that we have a StaticMutex so we can initialize it safely the first time it's used...but then we have InitializeGlobalState and DeInitializeGlobalState, which seem like perfect places to allocate and deallocate a mutex.  So why not have a normal Mutex, then?

Can we reach other functions that use gTelemetryScalarsMutex without first invoking InitializeGlobalState?  That seems...sketchy.  If you're going to use a StaticMutex, I think you need to explain the choice a little bit more.

@@ +477,5 @@
> +
> +bool
> +CanRecordBase() {
> +  StaticMutexAutoLock locker(gTelemetryScalarsMutex);
> +  return internal_CanRecordBase();

For these sorts of functions, I find it more useful to make them:

  bool CanRecordBase(const StaticMutexAutoLock&);

to both indicate that they are called with the lock held and to make it impossible to call them without such.

@@ +528,5 @@
> +  }
> +
> +  // Get the scalar info from the id.
> +  const uint32_t scalarIndex = static_cast<uint32_t>(id);
> +  const ScalarInfo &info = gScalars[scalarIndex];

I suggest a:

const ScalarInfo& InfoForScalarID(ScalarID);

function, assuming that:

  const ScalarInfo& info = gScalars[id];

doesn't work.  No sense in repeating yourself with scalarIndex over and over again.

@@ +536,5 @@
> +                                                              internal_CanRecordBase(),
> +                                                              internal_CanRecordExtended());
> +  if (!internal_CanRecordBase() || !canRecordDataset) {
> +    return NS_OK;
> +  }

Actually, given the only reason that you get a ScalarInfo is to do this check, maybe you should have:

  bool CanRecordForScalarID(ScalarID);

that calls the above InfoForScalarID, then use CanRecordForScalarID all over the place?  I guess maybe you need the scalar to be of a particular kind, but maybe you could figure out a way to roll that into CanRecordForScalarID...

  bool CanRecordForScalarID(ScalarID aID, const Maybe<ScalarIDKind>& aRequiredKind);

where aRequiredKind would be None if there was no constraint, and Some if there was?

Also, the check would be more efficient if you did:

  bool b = internal_CanRecordBase();
  if (!b) {
    return ...;
  }
  bool canRecordDataset = CanRecordDataset(info.dataset, b, internal_CanRecordExtended());
  ...

Maybe you expect the compiler to inline internal_CanRecordBase, but it seems helpful to simply reuse the value in any event?

Regardless, the middle part of this function is cut-and-pasted several times and we need to not do that.

Comment 20

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8762007 [details] [diff] [review]
bug1276195.patch

Review of attachment 8762007 [details] [diff] [review]:
-----------------------------------------------------------------

I only started to look at this, leaving the first comments here.

::: toolkit/components/telemetry/Telemetry.cpp
@@ +1935,5 @@
>                          XRE_IsParentProcess() || XRE_IsContentProcess(),
>                          XRE_IsParentProcess() || XRE_IsContentProcess());
>  
> +  // Only record scalars from the parent process.
> +  TelemetryScalar::InitializeGlobalState(XRE_IsParentProcess(), XRE_IsParentProcess());

Let's call this restriction out in the documentation.

::: toolkit/components/telemetry/TelemetryScalar.h
@@ +5,5 @@
> +
> +#ifndef TelemetryScalar_h__
> +#define TelemetryScalar_h__
> +
> +#include "mozilla/TelemetryScalarEnums.h"

Do we need to include this here?

@@ +25,5 @@
> +
> +// JS API Endpoints.
> +nsresult ScalarAdd(const nsACString &aName, JS::HandleValue aVal, JSContext *aCx);
> +nsresult ScalarSet(const nsACString &aName, JS::HandleValue aVal, JSContext *aCx);
> +nsresult ScalarSetMaximum(const nsACString &aName, JS::HandleValue aVal, JSContext *aCx);

Why not just `Add()`, `Set()`, `SetMaximum()`?

::: toolkit/components/telemetry/WebrtcTelemetry.h
@@ +36,5 @@
>  private:
>  
>    bool AddIceInfo(JSContext *cx, JS::Handle<JSObject*> rootObj, bool loop);
>  
> +  mozilla::Telemetry::Common::AutoHashtable<WebrtcIceCandidateType> mWebrtcIceCandidates;

This file doesn't actually include TelemetryCommon.h, lets fix that.

Comment 21

[Alessio Placitelli [:Dexter]]

Attachment: bug1276195.patch

Comment 22

[Alessio Placitelli [:Dexter]]

I'm clearing the ni? for MattN as we discussed this during the work week in London.

Comment 23

[Alessio Placitelli [:Dexter]]

(In reply to Nathan Froyd [:froydnj] from comment #19)
> Comment on attachment 8762007 [details] [diff] [review]
> bug1276195.patch
> 
> Review of attachment 8762007 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +466,5 @@
> > +
> > +void
> > +DeInitializeGlobalState()
> > +{
> > +  StaticMutexAutoLock locker(gTelemetryScalarsMutex);
> 
> Earlier you state that we have a StaticMutex so we can initialize it safely
> the first time it's used...but then we have InitializeGlobalState and
> DeInitializeGlobalState, which seem like perfect places to allocate and
> deallocate a mutex.  So why not have a normal Mutex, then?
> 
> Can we reach other functions that use gTelemetryScalarsMutex without first
> invoking InitializeGlobalState?  That seems...sketchy.  If you're going to
> use a StaticMutex, I think you need to explain the choice a little bit more.

Thank you Nathan for your comprehensive look at the patch and the useful feedback.

You are raising a good point here. Unfortunately, due to the nature of Telemetry, we cannot really guarantee that other functions won't be called before init takes place. I've added a comment near the StaticMutex to clear that out.

Comment 24

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8763585 [details] [diff] [review]
bug1276195.patch

Review of attachment 8763585 [details] [diff] [review]:
-----------------------------------------------------------------

First pass, looking at the tests only for a high-level functionality check (and test details).

::: toolkit/components/telemetry/TelemetryScalar.cpp
@@ +289,5 @@
> +
> +namespace {
> +
> +bool
> +CanRecordBase(const StaticMutexAutoLock&)

While this is one nice way to show that the lock is held, it is inconsistent here with the other functions.
I think the important part is that we have a consistent way to show which functions are internal and should be called only with a lock already held.

In `TelemetryHistogram.cpp` we have a big comment explaining that (please add one too) and prefix internal functions with `internal_`.
I don't think it's necessarily important whether we prefix or always pass the lock as an argument.
I think we should ask Julian on which way is more readable and easier to reason about from his recent experience.

::: toolkit/components/telemetry/tests/unit/test_TelemetryScalars.js
@@ +30,5 @@
> +  Assert.equal(scalars[STRING_SCALAR], expectedString,
> +               STRING_SCALAR + " must have the correct value.");
> +});
> +
> +add_task(function* test_unexistingScalar() {

`test_nonexistingScalar()`

@@ +37,5 @@
> +  Telemetry.clearScalars();
> +
> +  // Make sure we throw on any operation for non-existing scalars.
> +  Assert.throws(() => Telemetry.scalarAdd(NON_EXISTING_SCALAR, 11715),
> +                /NS_ERROR_INVALID_ARG|NS_ERROR_ILLEGAL_VALUE/,

We should know exactly which exception they throw and only check for that.

@@ +38,5 @@
> +
> +  // Make sure we throw on any operation for non-existing scalars.
> +  Assert.throws(() => Telemetry.scalarAdd(NON_EXISTING_SCALAR, 11715),
> +                /NS_ERROR_INVALID_ARG|NS_ERROR_ILLEGAL_VALUE/,
> +                "Setting a non existing scalar must throw.");

Update the message to mention add, not set.

@@ +44,5 @@
> +                /NS_ERROR_INVALID_ARG|NS_ERROR_ILLEGAL_VALUE/,
> +                "Setting a non existing scalar must throw.");
> +  Assert.throws(() => Telemetry.scalarSetMaximum(NON_EXISTING_SCALAR, 11715),
> +                /NS_ERROR_INVALID_ARG|NS_ERROR_ILLEGAL_VALUE/,
> +                "Setting a non existing scalar must throw.");

Update the message to mention SetMaximum.

@@ +62,5 @@
> +
> +  // Try to set the expired scalar to some value.
> +  Assert.throws(() => Telemetry.scalarSet(EXPIRED_SCALAR, 11715),
> +                /NS_ERROR_NOT_AVAILABLE/,
> +                "Setting an expired scalar must throw.");

Lets test both over the other operations too.

@@ +106,5 @@
> +  checkScalar(5);
> +  // The value of the probe should still be 5, as the previous value
> +  // is greater than the one we want to set.
> +  Telemetry.scalarSetMaximum(UINT_SCALAR, 3);
> +  checkScalar(5);

We need coverage for non-integral numbers too.

@@ +155,5 @@
> +  Assert.throws(() => Telemetry.scalarSetMaximum(STRING_SCALAR, "string value"),
> +                /NS_ERROR_NOT_AVAILABLE/,
> +                "Using an unsupported operation must throw.");
> +  Assert.throws(() => Telemetry.scalarSet(STRING_SCALAR, 1),
> +                /NS_ERROR_INVALID_ARG|NS_ERROR_ILLEGAL_VALUE/,

We should know which error this throws.

@@ +160,5 @@
> +                "The string scalar must throw if we're not setting a string.");
> +
> +  // Try to set the scalar to a string longer than the maximum length limit.
> +  const LONG_STRING = "browser.qaxfiuosnzmhlg.rpvxicawolhtvmbkpnludhedobxvkjwqyeyvmv";
> +  Assert.throws(() => Telemetry.scalarSet(STRING_SCALAR, LONG_STRING),

Should this actually throw?
This seems dangerous for the calling JS code - devs probably would not always (want to or remember to) check string lengths etc. and this exception scenario seems hard to discover.
I would suggest we just truncate the string and print a warning in the console.

@@ +225,5 @@
> +
> +  // Get a new snapshot and reset the subsession again. Since no new value
> +  // was set, the scalars should not be reported.
> +  scalars =
> +    Telemetry.snapshotScalars(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN);

`snapshotScalars()` should not default to resetting the subsession data, this seems unexpected.
Getting a snapshot is the more common use-case, and you should pass an explicit `true` to clear the subsession data.

Comment 25

[Georg Fritzsche [:gfritzsche]]

Matt, would it be unexpected for you for the scalar recording functions to throw on passing wrong types or using unsupported operations?

E.g., for a "unsigned int" scalar probe:
> Services.telemetry.scalarSet("group.probe", 1); // fine
> Services.telemetry.scalarSet("group.probe", "1"); // throws

... or for a string probe:
> Services.telemetry.scalarSet("group.probe", "x"); // fine
> Services.telemetry.scalarAdd("group.probe", 1); // throws
> Services.telemetry.scalarAdd("group.probe", "y"); // throws

If we are worried about exceptions breaking browser & toolkit code, we could just ignore these and log errors/warnings instead.
What do we commonly do for toolkit level interfaces like these?

Comment 26

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #24)
> Comment on attachment 8763585 [details] [diff] [review]
> bug1276195.patch
> 
> Review of attachment 8763585 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> First pass, looking at the tests only for a high-level functionality check
> (and test details).

Thanks!

> ::: toolkit/components/telemetry/TelemetryScalar.cpp
> @@ +289,5 @@
> > +
> > +namespace {
> > +
> > +bool
> > +CanRecordBase(const StaticMutexAutoLock&)
> 
> While this is one nice way to show that the lock is held, it is inconsistent
> here with the other functions.
> I think the important part is that we have a consistent way to show which
> functions are internal and should be called only with a lock already held.
> 
> In `TelemetryHistogram.cpp` we have a big comment explaining that (please
> add one too) and prefix internal functions with `internal_`.
> I don't think it's necessarily important whether we prefix or always pass
> the lock as an argument.
> I think we should ask Julian on which way is more readable and easier to
> reason about from his recent experience.

I think we could stick to the internal_* naming scheme to be consistent with what's in TelemetryHistogram.cpp.

> @@ +37,5 @@
> > +  Telemetry.clearScalars();
> > +
> > +  // Make sure we throw on any operation for non-existing scalars.
> > +  Assert.throws(() => Telemetry.scalarAdd(NON_EXISTING_SCALAR, 11715),
> > +                /NS_ERROR_INVALID_ARG|NS_ERROR_ILLEGAL_VALUE/,
> 
> We should know exactly which exception they throw and only check for that.

Yeah, that's a tricky one. Even though I'm throwing NS_ERROR_INVALID_ARG, I keep receiving NS_ERROR_ILLEGAL_VALUE in the xpcshell test (they are aliases). I resorted to throwing NS_ERROR_ILLEGAL_VALUE and just catching that in the tests, for clarity.



> @@ +106,5 @@
> > +  checkScalar(5);
> > +  // The value of the probe should still be 5, as the previous value
> > +  // is greater than the one we want to set.
> > +  Telemetry.scalarSetMaximum(UINT_SCALAR, 3);
> > +  checkScalar(5);
> 
> We need coverage for non-integral numbers too.

Should we just truncate in this case (instead of throwing)?

> @@ +160,5 @@
> > +                "The string scalar must throw if we're not setting a string.");
> > +
> > +  // Try to set the scalar to a string longer than the maximum length limit.
> > +  const LONG_STRING = "browser.qaxfiuosnzmhlg.rpvxicawolhtvmbkpnludhedobxvkjwqyeyvmv";
> > +  Assert.throws(() => Telemetry.scalarSet(STRING_SCALAR, LONG_STRING),
> 
> Should this actually throw?
> This seems dangerous for the calling JS code - devs probably would not
> always (want to or remember to) check string lengths etc. and this exception
> scenario seems hard to discover.

That's a very good point.

Comment 27

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8763585 [details] [diff] [review]
bug1276195.patch

Review of attachment 8763585 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/Telemetry.cpp
@@ +25,5 @@
>  #include "nsCOMArray.h"
>  #include "nsCOMPtr.h"
>  #include "nsXPCOMPrivate.h"
>  #include "nsIXULAppInfo.h"
> +#include "nsContentUtils.h"

Is this used?

@@ +52,5 @@
>  #include "nsClassHashtable.h"
>  #include "nsXULAppAPI.h"
>  #include "nsReadableUtils.h"
>  #include "nsThreadUtils.h"
> +#include "nsVariant.h"

Unused?

@@ +492,5 @@
>    };
>    typedef nsBaseHashtableET<nsStringHashKey, FileStatsByStage> FileIOEntryType;
>  
>    // Statistics for each filename
> +  Telemetry::Common::AutoHashtable<FileIOEntryType> mFileStats;

Given the repeated use here, a `using` declaration would make sense.

::: toolkit/components/telemetry/Telemetry.h
@@ +335,5 @@
> +/**
> + * Sets the scalar to the given value.
> + *
> + * @param aId The scalar enum id.
> + * @param aValue The string value to set the scalar to.

Document the string length limit.

::: toolkit/components/telemetry/TelemetryCommon.cpp
@@ +18,5 @@
> +{
> +  static mozilla::Version current_version = mozilla::Version(MOZ_APP_VERSION);
> +  MOZ_ASSERT(aExpiration);
> +  return strcmp(aExpiration, "never") && strcmp(aExpiration, "default") &&
> +    (mozilla::Version(aExpiration) <= current_version);

You need to include headers for strcmp or std::strcmp.
http://en.cppreference.com/mwiki/index.php?title=Special%3ASearch&search=strcmp

@@ +31,5 @@
> +
> +  // The "optin on release channel" dataset is a superset of the
> +  // "optout on release channel one".
> +  if (aContainingDataset == nsITelemetry::DATASET_RELEASE_CHANNEL_OPTIN
> +      && aDataset == nsITelemetry::DATASET_RELEASE_CHANNEL_OPTOUT) {

While we are moving this... Mozilla style is to have the `&&` trailing on the line before.

::: toolkit/components/telemetry/TelemetryScalar.cpp
@@ +85,5 @@
> +  ScalarUnsigned() : mStorage(0) {};
> +  ~ScalarUnsigned() {};
> +
> +  nsresult SetValue(nsIVariant* aValue) final;
> +  void SetValue(uint32_t aValue);

Why not define them as `NS_ERROR_NOT_AVAILABLE` in `ScalarBase` (and also `MOZ_ASSERT()` in them)?
Then we can override them here and don't need to do the - error prone - upcast below.
Ditto for the others and for `ScalarString`.

@@ +311,5 @@
> +
> +bool
> +CanRecordForScalarID(mozilla::Telemetry::ScalarID aId,
> +                     const mozilla::Maybe<uint32_t>& aRequiredKind,
> +                     const StaticMutexAutoLock& locker)

I think we should really do the type handling differently by declaring the typed methods in `ScalarBase`.

If we were to use that, we shouldn't use optional parameters and use overloading instead:
bool CanRecordForScalarID(id);
bool CanRecordForScalarID(id, kind); // Calls the former and also type checks.

@@ +318,5 @@
> +  const ScalarInfo &info = InfoForScalarID(aId);
> +
> +  // This needs to be of the required type.
> +  if (aRequiredKind && info.kind != *aRequiredKind) {
> +    return false;

We should MOZ_ASSERT() these things.

@@ +381,5 @@
> + *   NS_ERROR_INVALID_ARG if the name can't be found in the scalar definitions.
> + *   NS_OK if the scalar was found and aId contains a valid enum id.
> + */
> +nsresult
> +GetEnumByScalarName(const char* aName, mozilla::Telemetry::ScalarID* aId)

All callers actually have some kind of `nsCString` for the name.
Make this a `const nsCString&` and do conversion as needed internally.

@@ +485,5 @@
> +// NOTE: All the functions in the TelemetryScalar namespace are thread-safe.
> +namespace TelemetryScalar {
> +
> +void
> +InitializeGlobalState(bool aCanRecordBase, bool aCanRecordExtended)

Julian wrote all of the public-facing functions as `TelemetryHistogram::...` instead of defining them in the namespace, so it's clear they are part of the public interface.
Let's mimic that here as `TelemetryScalar::Initialize...` etc.

@@ +499,5 @@
> +  // statically allocated and come from the automatically generated TelemetryScalarData.h.
> +  uint32_t scalarCount = static_cast<uint32_t>(mozilla::Telemetry::ScalarID::ScalarCount);
> +  for (uint32_t i = 0; i < scalarCount; i++) {
> +    CharPtrEntryType *entry = gScalarNameIDMap.PutEntry(gScalars[i].name());
> +    entry->mData = (mozilla::Telemetry::ScalarID) i;

static_cast

::: toolkit/components/telemetry/TelemetryScalar.h
@@ +7,5 @@
> +#define TelemetryScalar_h__
> +
> +#include "mozilla/TelemetryScalarEnums.h"
> +
> +class nsIVariant;

Unused.

::: toolkit/components/telemetry/docs/scalars.rst
@@ +1,3 @@
>  =======
>  Scalars
>  =======

Lets add some interface documentation to this file - i'm happy to take this to a follow-up bug though.
Ideally we could include documentation to include from the doc comments here, but not now.
However, we could at least document the basics of the IDL and C++ interfaces, from there devs can look up more details in the `.idl` / `.h`.

::: toolkit/components/telemetry/nsITelemetry.idl
@@ +397,5 @@
> +   *
> +   * @param aName The scalar name.
> +   * @param aValue The value to set the scalar to. Only unsigned integers supported. If
> +   *        the type of aValue doesn't match the type of the scalar, the function will
> +   *        fail.

Document the string length limit.

@@ +412,5 @@
> +  [implicit_jscontext]
> +  void scalarSetMaximum(in ACString aName, in jsval aValue);
> +
> +  /**
> +   * Serializes the scalars from the given dataset to a json-style object and resets them.

Nit: "JSON-style"

@@ +417,5 @@
> +   * The returned structure looks like:
> +   *   { "group1.probe": 1, "group1.other_probe": false, ... }
> +   *
> +   * @param aDataset DATASET_RELEASE_CHANNEL_OPTOUT or DATASET_RELEASE_CHANNEL_OPTIN.
> +   * @param [aClear=true] Whether to clear out the scalars after snapshotting.

This should not default to `true`.

Comment 28

[Alessio Placitelli [:Dexter]]

Attachment: bug1276195.patch

Comment 29

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8764298 [details] [diff] [review]
bug1276195.patch

Review of attachment 8764298 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/TelemetryScalar.cpp
@@ +72,5 @@
> +enum class ScalarResult : uint8_t {
> +  // Nothing went wrong.
> +  Ok,
> +  // General Scalar Errors
> +  NotInitialized,

This, UnknownScalarId, UnknownScalarName and UnknownScalarType seem unrelated/orthogonal to the other status codes.
The other codes are about value changing operations on existing scalars, we should keep it to that.

@@ +112,5 @@
> +      return NS_ERROR_ILLEGAL_VALUE;
> +    case ScalarResult::UnknownScalarName:
> +      return NS_ERROR_ILLEGAL_VALUE;
> +    case ScalarResult::UnknownScalarType:
> +      return NS_ERROR_ILLEGAL_VALUE;

You should make those fall-through too, all three return the same value.

@@ +157,5 @@
> +class ScalarBase
> +{
> +public:
> +  virtual ~ScalarBase() {};
> +  // Set, Add and SetMaximum functions as described in the Telemetry IDL.

Readability nit: Empty line before this and the three other "section" comments below.

@@ +162,5 @@
> +  virtual ScalarResult SetValue(nsIVariant* aValue) = 0;
> +  virtual ScalarResult AddValue(nsIVariant* aValue) { return ScalarResult::OperationNotSupported; }
> +  virtual ScalarResult SetMaximum(nsIVariant* aValue) { return ScalarResult::OperationNotSupported; }
> +  // Convenience methods used by the C++ API.
> +  virtual void SetValue(uint32_t aValue) { HandleUnsupported(); }

You will probably get warnings for ignored return values here.

@@ +167,5 @@
> +  virtual ScalarResult SetValue(const nsAString& aValue) { return HandleUnsupported(); }
> +  virtual void AddValue(uint32_t aValue) { HandleUnsupported(); }
> +  virtual void SetMaximum(uint32_t aValue) { HandleUnsupported(); }
> +  // GetValue is used to get the value of the scalar when persisting it to JS.
> +  virtual ScalarResult GetValue(nsCOMPtr<nsIVariant>& aResult) = 0;

`GetValue()`, `SizeOf..()` and `HandleUnsupported()` should be `const`.

@@ +178,5 @@
> +
> +ScalarResult
> +ScalarBase::HandleUnsupported()
> +{
> +  MOZ_ASSERT(false, "Unsupported scalar operation.");

"This operation is not support for this scalar type"?

@@ +199,5 @@
> +  ScalarResult AddValue(nsIVariant* aValue) final;
> +  void AddValue(uint32_t aValue) final;
> +  ScalarResult SetMaximum(nsIVariant* aValue) final;
> +  void SetMaximum(uint32_t aValue) final;
> +  ScalarResult GetValue(nsCOMPtr<nsIVariant>& aResult) final;

The results of this operation are unrelated to the Set/Add/... operations.
Use a different return type.

@@ +288,5 @@
> +{
> +  nsCOMPtr<nsIWritableVariant> outVar(new nsVariant());
> +  nsresult rv = outVar->SetAsUint32(mStorage);
> +  if (NS_FAILED(rv)) {
> +    return ScalarResult::UnsignedInvalidValue;

I don't think that status code makes sense here.
What are the reasons this could fail?
Maybe this should just return some general error.

@@ +314,5 @@
> +  }
> +  return ScalarResult::Ok;
> +}
> +
> +const uint32_t kMaximumStringValueLength = 50;

This is rather hidden.
Let's put constants like this at the top.

@@ +358,5 @@
> +      type != nsIDataType::VTYPE_CSTRING &&
> +      type != nsIDataType::VTYPE_ASTRING) {
> +    return ScalarResult::StringInvalidType;
> +  }
> +  nsAutoString convertedString;

Nit: Empty line before this.

@@ +414,5 @@
> +
> +bool gCanRecordBase;
> +bool gCanRecordExtended;
> +
> +const uint32_t kStorageSize =

Why not `kScalarCount`?

@@ +458,5 @@
> +      NS_NewRunnableFunction([aLogLevel, msg]() { internal_LogToBrowserConsole(aLogLevel, msg); });
> +    NS_DispatchToMainThread(task.forget(), NS_DISPATCH_NORMAL);
> +    return;
> +  }
> +  nsCOMPtr<nsIConsoleService> console(do_GetService("@mozilla.org/consoleservice;1"));

Nit: Empty line before this.

@@ +463,5 @@
> +  if (!console) {
> +    NS_WARNING("Failed to log message to console.");
> +    return;
> +  }
> +  nsCOMPtr<nsIScriptError> error(do_CreateInstance(NS_SCRIPTERROR_CONTRACTID));

Nit: Empty line before this.

@@ +486,5 @@
> +        AppendUTF8toUTF16(aScalarName, errorMessage);
> +        errorMessage.Append(NS_LITERAL_STRING(" - Truncating scalar value to 50 characters."));
> +        internal_LogToBrowserConsole(nsIScriptError::warningFlag, errorMessage);
> +      }
> +      break;

Nit: Have the `break` inside the braces, then you save one indentation level. See the style guide.

@@ +562,5 @@
> + *   this produces an assertion.
> + *   ScalarResult::Ok if the scalar was allocated. If that's the case, aResult contains a
> + *   valid pointer to a scalar type.
> + */
> +ScalarResult

The results of this function (and the others following below) are unrelated to the scalar set/add/... operations and should not use the same result code.
The `ScalarResult` codes are useful as a focused status code for the scalar operations, i.e. for the `ScalarBase` interface. Using them in other places is not necessary.

You don't log on either of the two specific return codes, so why not just return `ScalarBase*` and `nullptr` for failure?

@@ +577,5 @@
> +  case nsITelemetry::SCALAR_STRING:
> +    *aResult = new ScalarString();
> +    break;
> +  default:
> +    NS_ASSERTION(false, "Invalid scalar type");

Why `NS_ASSERTION()` here vs. `MOZ_ASSERT()` elsewhere?

@@ +591,5 @@
> + * @param aId The output variable to contain the enum.
> + * @return
> + *   ScalarResult::NotInitialized if this was called before init is completed.
> + *   ScalarResult::UnknownScalarName if the name can't be found in the scalar definitions.
> + *   ScalarResult::Ok if the scalar was found and aId contains a valid enum id.

Ditto, just return `bool` or nsresults?

@@ +619,5 @@
> + *   ScalarResult::UnknownScalarId if the scalar id is unknown.
> + *   ScalarResult::Ok if the scalar was found. If that's the case, aResult contains a
> + *   valid pointer to a scalar type.
> + */
> +ScalarResult

Just return `ScalarBase*` and `nullptr` for "unknown id".

@@ +759,5 @@
> +
> +    sr = scalar->AddValue(unpackedVal);
> +  }
> +
> +  internal_LogScalarError(aName, sr);

It's confusing to always call a "log error" function here.

::: toolkit/components/telemetry/tests/unit/test_TelemetryScalars.js
@@ +112,5 @@
> +  checkScalar(5);
> +
> +  // Check that non-integer numbers get truncated and set.
> +  Telemetry.scalarSet(UINT_SCALAR, 3.785);
> +  checkScalar(3);

Do we log a warning or error for this too?

Comment 30

[Alessio Placitelli [:Dexter]]

Attachment: bug1276195.patch

Comment 31

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] [away Jun 24 - Jul 3] from comment #29)
> Comment on attachment 8764298 [details] [diff] [review]
> bug1276195.patch
> 
> Review of attachment 8764298 [details] [diff] [review]:
> -----------------------------------------------------------------

> @@ +162,5 @@
> > +  virtual ScalarResult SetValue(nsIVariant* aValue) = 0;
> > +  virtual ScalarResult AddValue(nsIVariant* aValue) { return ScalarResult::OperationNotSupported; }
> > +  virtual ScalarResult SetMaximum(nsIVariant* aValue) { return ScalarResult::OperationNotSupported; }
> > +  // Convenience methods used by the C++ API.
> > +  virtual void SetValue(uint32_t aValue) { HandleUnsupported(); }
> 
> You will probably get warnings for ignored return values here.

Odd that I didn't get them :-/ I've changed that to be safe!

> @@ +619,5 @@
> > + *   ScalarResult::UnknownScalarId if the scalar id is unknown.
> > + *   ScalarResult::Ok if the scalar was found. If that's the case, aResult contains a
> > + *   valid pointer to a scalar type.
> > + */
> > +ScalarResult
> 
> Just return `ScalarBase*` and `nullptr` for "unknown id".

I've changed that in all the places except |internal_GetScalarByEnum|, where we use the different error codes to distinguish the "expired scalar" case.

> ::: toolkit/components/telemetry/tests/unit/test_TelemetryScalars.js
> @@ +112,5 @@
> > +  checkScalar(5);
> > +
> > +  // Check that non-integer numbers get truncated and set.
> > +  Telemetry.scalarSet(UINT_SCALAR, 3.785);
> > +  checkScalar(3);
> 
> Do we log a warning or error for this too?

Good catch, we want and now we do.

Comment 32

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8764653 [details] [diff] [review]
bug1276195.patch

Review of attachment 8764653 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, this looks good to me!

::: toolkit/components/telemetry/TelemetryScalar.cpp
@@ +501,5 @@
> +{
> +  nsAutoString errorMessage;
> +  AppendUTF8toUTF16(aScalarName, errorMessage);
> +
> +  if (aSr == ScalarResult::StringTooLong) {

Make this a switch statement.

Comment 33

[Alessio Placitelli [:Dexter]]

Attachment: bug1276195.patch

Comment 34

[Alessio Placitelli [:Dexter]]

Attachment: bug1276195.patch

Fixes the bustage from https://treeherder.mozilla.org/#/jobs?repo=try&revision=f73fbfa17e56&selectedJob=22877199 and does a little of refactoring.

Comment 35

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/33fbfeeb7e4eb54824d825aebb81e61916520a78
Bug 1276195 - Add scalar measurements API & storage in Telemetry C++ core. r=gfritzsche

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/33fbfeeb7e4e