Closed Bug 1342090 Opened 3 years ago Closed 3 years ago

Update SSL_HANDSHAKE_RESULT to opt-out on release

Categories

(Core :: Security: PSM, defect, P3)

39 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: ekr, Assigned: ekr)

Details

Attachments

(1 file)

Attached patch telemetry.diffSplinter Review
This is a basic piece of health information about the browser, and release has a very different population, so I would like to make it opt-out. The information it reports is just the frequency of different types of SSL/TLS errors, but not the details of the errors themselves. E.g., it tells you that you have a certificate error of type X but not the certificate you saw.
Attachment #8840461 - Flags: review?(benjamin)
Summary: Update SSL_HANDSHAKE_VERSION to opt-out on release → Update SSL_HANDSHAKE_RESULT to opt-out on release
Assignee: nobody → ekr
Priority: -- → P3
Component: Telemetry → Security: PSM
Product: Toolkit → Core
Benjamin,

Sorry if I put this in the wrong component. I assumed since it was just a change to Histograms.json it was in telemetry. In any case, it needs your review :)
Comment on attachment 8840461 [details] [diff] [review]
telemetry.diff

Do we have automation to make sure this metric still works as expected? That is typically requested as risk mitigation of opt-out probes.

Also who exactly is committing to use this data? ekr is that you personally, or is that a team (or is there a PM?)
Flags: needinfo?(ekr)
(In reply to Benjamin Smedberg [:bsmedberg] from comment #2)
> Comment on attachment 8840461 [details] [diff] [review]
> telemetry.diff
> 
> Do we have automation to make sure this metric still works as expected? That
> is typically requested as risk mitigation of opt-out probes.

Can you elaborate on this? We don't have any, but I'm happy to write some if I have
a better sense of what is needed.


> Also who exactly is committing to use this data? ekr is that you personally,
> or is that a team (or is there a PM?)

Me like pretty much the day it ships. We are trying to understand why we are seeing
increased errors on TLS 1.3 and this is for diagnosing that.
Flags: needinfo?(ekr)
Comment on attachment 8840461 [details] [diff] [review]
telemetry.diff

data-r=me for now and let's do a followup for tests. Expect me to ping you every 6 months or so to make sure you're still using this. A typical telemetry test progressively runs a bunch of actions that should trigger telemetry and then checks that the histogram records what we expect. https://reviewboard.mozilla.org/r/78732/diff/7#index_header and bug 1301452 has an example of a test for tab-switch-spinner probes.
Attachment #8840461 - Flags: review?(benjamin) → review+
Whiteboard: checkin-needed
Comment on attachment 8840461 [details] [diff] [review]
telemetry.diff

Approval Request Comment
[Feature/Bug causing the regression]: Final patch in sequence for reasonable TLS telemetry
[User impact if declined]: Bad visibility on TLS 1.3
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: No. I built it and verified that it didn't break stuff and then marked it checkin-needed. It's just a telemetry setting, but if we wanted we could miss b9 and drop it for RC1. This seems like a judgement call, but I do want it on FF52.
[Needs manual test from QE? If yes, steps to reproduce]: Do a build with release settings and make sure that SSL_HANDSHAKE_RESULT gets sent in telemetry.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Minimally.
[Why is the change risky/not risky?]: Telemetry setting frob
[String changes made/needed]:
Flags: needinfo?(jcristau)
Attachment #8840461 - Flags: approval-mozilla-beta?
Attachment #8840461 - Flags: approval-mozilla-aurora?
Thanks Benjamin. I filed this as: 1342090
Comment on attachment 8840461 [details] [diff] [review]
telemetry.diff

Improvements for tracking TLS 1.3 issues, let's uplift now. This may or may not make today's build, but it should make it ok to the RC build on Monday (though if it misses then we need to uplift to m-r)
Attachment #8840461 - Flags: approval-mozilla-beta?
Attachment #8840461 - Flags: approval-mozilla-beta+
Attachment #8840461 - Flags: approval-mozilla-aurora?
Attachment #8840461 - Flags: approval-mozilla-aurora+
Thank you everyone!
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a585de719c2
Upgrade SSL_HANDSHAKE_RESULT to opt-out on release. r=bsmedberg
Flags: needinfo?(jcristau)
https://hg.mozilla.org/mozilla-central/rev/3a585de719c2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.