Closed Bug 1330907 Opened 7 years ago Closed 7 years ago

Rename Telemetry::ID to Telemetry::HistogramID

Categories

(Toolkit :: Telemetry, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- affected
firefox54 --- fixed

People

(Reporter: Dexter, Assigned: fionn_mac, Mentored)

References

Details

(Whiteboard: [measurement:client] [lang=c++])

Attachments

(1 file, 2 obsolete files)

Depends on: 1278556
Priority: -- → P2
Whiteboard: [measurement:client]
Mentor: alessio.placitelli
Whiteboard: [measurement:client] → [measurement:client] [lang=c++]
Vedant, would you be interested in taking this bug?
Flags: needinfo?(vedant.sareen)
(In reply to Alessio Placitelli [:Dexter] from comment #1)
> Vedant, would you be interested in taking this bug?

I'll be glad to!
I'll start work on this ASAP :)
Flags: needinfo?(vedant.sareen)
(In reply to Vedant Sareen from comment #2)
> (In reply to Alessio Placitelli [:Dexter] from comment #1)
> > Vedant, would you be interested in taking this bug?
> 
> I'll be glad to!
> I'll start work on this ASAP :)

Great! :-D Thank you!
Assignee: nobody → vedant.sareen
Priority: P2 → P3
Terribly sorry for the delay.
I was unable to complete the first patch as I got hard pressed between college assignments and (now ongoing) exams.
I'll try my best to upload the proposed patch by Thursday evening.
(In reply to Vedant Sareen from comment #4)
> Terribly sorry for the delay.
> I was unable to complete the first patch as I got hard pressed between
> college assignments and (now ongoing) exams.
> I'll try my best to upload the proposed patch by Thursday evening.

No problem, but thanks for keeping us posted!
Sir, I was working on replacing mozilla::Telemetry::ID to mozilla::Telemetry::HistogramID, but now I am getting errors that HistogramID is not a member of mozilla::Telemetry namespace, which is something that I should have expected because I didn't touch the actual namespace itself. 
I believe changing ID to HistogramId in the namespace will fix the issue.

I couldn't find the file which might contain the definitions of Telemetry namespace though. Could you please help me with that?

On a side note, initially I had tried replacing the mentioned string simply by running the script
> find . -type f -exec sed -i 's/Telemetry::ID/Telemetry::HistogramID/g' {} + 

However, I wasn't able to view the diff or build after doing so, as each time I got the error -

> abort: index data/image/decoders/nsGIFDecoder2.h.i is corrupted!

From what I understand, .i files are simply pre-processed C/C++ files, and since I'm using an artifact build, such files are expected. How can they get corrupted though?

I made all the changes manually the next time, just to be sure that I wasn't doing anything wrong.
Flags: needinfo?(alessio.placitelli)
Attached patch bug1330907.patch (obsolete) — Splinter Review
First proposed patch for Bug 1330907.

I was able to build successfully after making the above mentioned changes.
The earlier errors were due to certain forward references that I had failed to notice earlier, which made me think that Telemetry namespace had been defined elsewhere and not just |toolkit/components/telemetry/gen-histogram-enum.py|. They have been resolved now.

I haven't run any python/cpp unit tests yet, as running mach cppunittest/gtest runs 0 tests. I did run mochitest though. All tests were successful.
Flags: needinfo?(alessio.placitelli)
Attachment #8836324 - Flags: review?(alessio.placitelli)
Comment on attachment 8836324 [details] [diff] [review]
bug1330907.patch

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

Thank you for the patch and the efforts! This looks good overall, there are just a couple of indentation issues and a few wrong comments.

Please do not strip whitespaces throughout the files unless they are on a line of code you'd be changing anyway. Most of the review comments are about that.

::: dom/base/nsDocument.cpp
@@ +12572,1 @@
>                                          uc * 2 + 1);

nit: please keep |uc * 2 + 1| aligned to the |Telemetry::| part on the line above.

::: dom/media/eme/DetailedPromise.cpp
@@ +100,1 @@
>                                               : mFailureLatencyProbe.Value();

nit: please make sure |: mFailureLatency ..| aligns with the question mark on the line above.

::: dom/storage/StorageCache.cpp
@@ +59,4 @@
>  
>  NS_IMPL_ADDREF(StorageCacheBridge)
>  
> +// Since there is no consumer of return value of Release, we can turn this

nit: please drop this change (even if it's good to remove trailing whitespaces, it would clutter the history)

@@ +149,4 @@
>    // Check the quota string has (or has not) the identical origin suffix as
>    // this storage cache is bound to.
>    MOZ_ASSERT(StringBeginsWith(mQuotaOriginScope, mOriginSuffix));
> +  MOZ_ASSERT(mOriginSuffix.IsEmpty() != StringBeginsWith(mQuotaOriginScope,

nit: please drop this change (even if it's good to remove trailing whitespaces, it would clutter the history)

::: dom/storage/StorageCache.h
@@ +78,5 @@
>  public:
>    NS_IMETHOD_(void) Release(void);
>  
> +  // Note: We pass aOriginNoSuffix through the ctor here, because
> +  // StorageCacheHashKey's ctor is creating this class and

nit: please drop these changes (even if it's good to remove trailing whitespaces, it would clutter the history)

@@ +211,4 @@
>    // The origin attributes suffix
>    nsCString mOriginSuffix;
>  
> +  // The eTLD+1 scope used to count quota usage.  It is in the reversed format

nit: please drop this change (even if it's good to remove trailing whitespaces, it would clutter the history)

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
@@ +284,1 @@
>                              WEBRTC_VIDEO_QUALITY_OUTBOUND_RTT;

Please align WEBRTC_VIDEO_QUALITY_OUTBOUND_RTT with the WEBRTC_* above.

::: netwerk/base/nsSocketTransport2.cpp
@@ +3222,4 @@
>  
>  void
>  nsSocketTransport::SendPRBlockingTelemetry(PRIntervalTime aStart,
> +                                           Telemetry::HistogramID aIDNormal,

Please drop all the other whitespaces changes in this file.

::: netwerk/cache/nsCacheService.cpp
@@ +2649,2 @@
>  {
> +    mozilla::Telemetry::HistogramID lockerID;

Please drop all the whitespaces changes to this file.

::: netwerk/cache2/CacheIOThread.cpp
@@ +49,4 @@
>      return;
>    }
>  
> +  static Telemetry::HistogramID telemetryID[] = {

Please drop all the whitespaces changes to this file.

::: netwerk/dns/nsHostResolver.cpp
@@ +1481,4 @@
>                  uint32_t millis = static_cast<uint32_t>(elapsed.ToMilliseconds());
>  
>                  if (NS_SUCCEEDED(status)) {
> +                    Telemetry::HistogramID histogramID;

Please drop all the whitespaces changes to this file.

::: security/manager/ssl/RootCertificateTelemetryUtils.cpp
@@ +76,4 @@
>  // Attempt to increment the appropriate bin in the provided Telemetry probe ID. If
>  // there was a hash failure, we do nothing.
>  void
> +AccumulateTelemetryForRootCA(mozilla::Telemetry::HistogramID probe, 

You can remove the trailing whitespace from here since you're changing the line anyway.

::: security/manager/ssl/nsNSSCallbacks.cpp
@@ +39,4 @@
>  
>  extern LazyLogModule gPIPNSSLog;
>  
> +static void AccumulateCipherSuite(Telemetry::HistogramID probe,

Please drop all the whitespaces changes to this file.

::: storage/TelemetryVFS.cpp
@@ +37,4 @@
>  
>  struct Histograms {
>    const char *name;
> +  const Telemetry::HistogramID readB;

Please drop all the whitespaces changes to this file.

::: toolkit/components/places/Helpers.h
@@ +276,4 @@
>  class AsyncStatementTelemetryTimer : public AsyncStatementCallback
>  {
>  public:
> +  explicit AsyncStatementTelemetryTimer(Telemetry::HistogramID aHistogramId,

Please drop all the whitespaces changes in this file.

::: toolkit/components/telemetry/Telemetry.cpp
@@ +2723,4 @@
>  void
>  XRE_TelemetryAccumulate(int aID, uint32_t aSample)
>  {
> +  mozilla::Telemetry::Accumulate((mozilla::Telemetry::HistogramID) aID, aSample);

Please drop all the whitespaces changes in this file.

::: toolkit/components/telemetry/Telemetry.h
@@ +161,4 @@
>  
>  /**
>   * Enable/disable recording for this histogram at runtime.
> + * Recording is enabled by default, unless listed at kRecordingInitiallyDisabledHistogramIDs[].

Please restore this line.

::: widget/nsIdleService.cpp
@@ +378,4 @@
>  ////////////////////////////////////////////////////////////////////////////////
>  //// nsIdleService
>  
> +namespace {

Please drop all changes in this file, as no HistogramID is being used.
Attachment #8836324 - Flags: review?(alessio.placitelli) → feedback+
Status: NEW → ASSIGNED
(In reply to Vedant Sareen from comment #7)
> I haven't run any python/cpp unit tests yet, as running mach
> cppunittest/gtest runs 0 tests. I did run mochitest though. All tests were
> successful.

Nice! The python and cpp changes are indirectly covered by the mochitest, so we should be ok. Unfortunately, we don't have gtest c++ test coverage for the Histogram API right now.
(In reply to Alessio Placitelli [:Dexter] from comment #8)

I've fixed most of the unwanted changes. Only a few are remaining, as I have doubt regarding how to fix them.

> ::: dom/base/nsDocument.cpp
> @@ +12572,1 @@
> >                                          uc * 2 + 1);
> 
> nit: please keep |uc * 2 + 1| aligned to the |Telemetry::| part on the line
> above.

Sir, the |uc * 2 + 1| part was on a separate line earlier as well, as per http://searchfox.org/mozilla-central/source/dom/base/nsDocument.cpp#12574 .
It simply isn't coinciding with the beginning of |(Telemetry::HistogramFirstUseCounter +| anymore, and bringing it to the upper line increases the line character count to 91 (99 including the indentation at the beginning).
Should I keep it in a separate line but make it coincide with the beginning of the bracket above, or should I align it with the |Telemetry::| part?

> ::: dom/media/eme/DetailedPromise.cpp
> @@ +100,1 @@
> >                                               : mFailureLatencyProbe.Value();
> 
> nit: please make sure |: mFailureLatency ..| aligns with the question mark
> on the line above.

Same issue as above, as per http://searchfox.org/mozilla-central/source/dom/media/eme/DetailedPromise.cpp#99

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
> @@ +284,1 @@
> >                              WEBRTC_VIDEO_QUALITY_OUTBOUND_RTT;
> 
> Please align WEBRTC_VIDEO_QUALITY_OUTBOUND_RTT with the WEBRTC_* above.

Same issue as above, as per http://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp#283

I am uploading an initial patch in which I have aligned these pieces of text with the respective brackets (or other symbols that they were earlier aligned with) in the lines above while keeping them in separate lines, in accordance with the 80 character rule. In case changes are required, I'll do the needful and upload the other version :)
Flags: needinfo?(alessio.placitelli)
Attachment #8836324 - Attachment is obsolete: true
Attachment #8837359 - Flags: review?(alessio.placitelli)
Comment on attachment 8837359 [details] [diff] [review]
Hopefully fixed patch for Bug 1330907

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

Thank you Vedant!

The patch is good, you just introduced a trailing whitespace in nDocument.cpp. Please remove the whitespace in the marked location only, and we're good to go!

::: dom/base/nsDocument.cpp
@@ +12568,4 @@
>        }
>  
>        if (IsTopLevelContentDocument()) {
> +        id = static_cast<Telemetry::HistogramID>(Telemetry::HistogramFirstUseCounter + 

nit: please remove the trailing space in this line :)
Attachment #8837359 - Flags: review?(alessio.placitelli) → feedback+
Flags: needinfo?(alessio.placitelli)
Attachment #8837359 - Attachment is obsolete: true
Attachment #8837732 - Flags: review?(alessio.placitelli)
Attachment #8837732 - Flags: review?(alessio.placitelli) → review+
Vedant, thank you for your patch! I've pushed it to try for you.
https://hg.mozilla.org/mozilla-central/rev/abdc71fec220
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(In reply to Alessio Placitelli [:Dexter] from comment #15)
> Vedant, thank you for your patch! I've pushed it to try for you.

Thank you for letting me work on these bugs :)
You need to log in before you can comment on or make changes to this bug.