Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Add telemetry probes #4665

Merged
merged 1 commit into from
Jan 24, 2018
Merged

Add telemetry probes #4665

merged 1 commit into from
Jan 24, 2018

Conversation

jasonLaster
Copy link
Contributor

Associated Issue: #4158

Summary of Changes

  • needs shims from devtools-core
  • needs some sync w/ releasing so we can have the histograms/scalar.yml work done

@codehag
Copy link
Contributor

codehag commented Nov 24, 2017

it isnt quite clear to me what needs to be done to pick this up, let me know when you have a minute!

@jasonLaster
Copy link
Contributor Author

@codehag i added the additional yaml and json files,

I recommend talking with @juliandescottes when it comes time to do the release and land the updated files and get the data review.

bwinton
bwinton reviewed Nov 28, 2017
View reviewed changes
assets/panel/Histograms.json Outdated
"kind": "exponential",
"high": 10000,
"n_buckets": 1000,
"description": "The time (in milliseconds) that it took to load a source for the user."
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like Type-1 or Type-2 data. Do we want it to be opt-in or opt-out on the Release channel?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too sure... how are we handling this on other tools? @juliandescottes ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most likely type 2 data category (data collection categories).

Regarding opt-in/opt-out, I have never explicitly defined this in Histograms.json, so not sure either. There has been a recent data collection policy change (link). Don't know if we can still use the releaseChannelCollection property for new probes?

Let's ask @fmarier

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks like Category1/2, which is allowed to be opt-out on all channels (including release).

codehag
codehag reviewed Nov 29, 2017
View reviewed changes
src/test/mochitest/browser_dbg-preview.js Outdated
@@ -65,4 +65,7 @@ add_task(async function() {
await popupPreviewed;
assertPopup(dbg, { field: "foo", value: "1", expression: "obj" });
assertPopup(dbg, { field: "bar", value: "2", expression: "obj" });

await waitForLoadedObjects(dbg)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this part of the work for adding telemetry probes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know either but it would be nice to clean it up before merging?

codehag
codehag reviewed Nov 29, 2017
View reviewed changes
src/actions/sources/loadSourceText.js Outdated
@@ -47,6 +53,9 @@ export function loadSourceText(source: Source) {
return;
}

const delay = 300;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to have a calculated value here?

@juliandescottes
Copy link
Member

I'm not sure about the best process here to add probes for the debugger. Histograms.json and scalars.yml are both files that are not debugger (or even devtools) specific, and I'm not sure about the impact if those files become out of sync with the artifacts used for testing.

It might be easier to land probes in mozilla-central separately, and then merge the implementation in the debugger after the probes have landed.

@codehag
Copy link
Contributor

codehag commented Jan 9, 2018

From discussing things with julian and mike, i am not sure we will use this patch, is it alright if i close it @jasonLaster ?

@jasonLaster jasonLaster closed this Jan 9, 2018
@codehag codehag reopened this Jan 11, 2018
@codehag codehag closed this Jan 11, 2018
@codehag codehag reopened this Jan 11, 2018
@codehag codehag force-pushed the add-telemetry-probes branch 2 times, most recently from eb3b6a7 to 3b0e3ab Compare January 12, 2018 12:45
juliandescottes
juliandescottes approved these changes Jan 12, 2018
View reviewed changes
Copy link
Member

@juliandescottes juliandescottes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments, most importantly the name of the probe needs to be fixed, but looks good otherwise.

src/actions/sources/loadSourceText.js Outdated
@@ -22,6 +22,10 @@ import type { SourceRecord } from "../../reducers/types";
import { searchSource } from "../project-text-search";

const requests = new Map();
import { Services } from "devtools-modules";
const loadSourceHistogram = Services.telemetry.getHistogramById(
"DEVTOOLS_DEBUGGER_LOAD_SOURCE"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name should be DEVTOOLS_DEBUGGER_LOAD_SOURCE_MS

src/actions/sources/loadSourceText.js Outdated
@@ -103,5 +108,9 @@ export function loadSourceText(source: SourceRecord) {
// signal that the action is finished
deferred.resolve();
requests.delete(id);

const TELEMETRY_END = performance.now();
const duration = TELEMETRY_END - TELEMETRY_START;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit: not sure about the coding conventions in the project, but all uppercase for telemetry start/end feels a bit weird. I would expect all uppercase to be file constants.

src/test/mochitest/browser_dbg-preview.js Outdated
@@ -65,4 +65,7 @@ add_task(async function() {
await popupPreviewed;
assertPopup(dbg, { field: "foo", value: "1", expression: "obj" });
assertPopup(dbg, { field: "bar", value: "2", expression: "obj" });

await waitForLoadedObjects(dbg)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know either but it would be nice to clean it up before merging?

docs/local-development.md Outdated
```

When you add a new scalar or histogram, you'll need to add it to the
[local histograms.json][localhistograms.json] or [local scalars.yml][localscalars.yaml]. This needs to be moved to MC,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation should no longer mention the local histrograms/scalars files. Can point to our current Bug as an example, and mention that the probes can be landed in mozilla-central before performing the implementation in the debugger.

@juliandescottes
Copy link
Member

Ok, so we are failing CI for now, since the probes are not available in the build of m-c we are using for the tests. Either we wait for the probe to land and we update the revision of m-c being used for tests. Or we try/catch around it.

jasonLaster
jasonLaster commented Jan 12, 2018
View reviewed changes
Copy link
Contributor Author

@jasonLaster jasonLaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@codehag codehag changed the title (WIP) Add telemetry probes Add telemetry probes Jan 12, 2018
@juliandescottes
Copy link
Member

juliandescottes commented Jan 18, 2018 •
edited
Loading

Probe landed in https://hg.mozilla.org/mozilla-central/rev/55b95957d30b

Need to update https://github.com/devtools-html/debugger.html/blob/master/.travis.yml#L13

@jasonLaster
Copy link
Contributor Author

@juliandescottes hmm still doesnt see the ID...

@juliandescottes
Copy link
Member

For whatever reason it seems that using "just" the first rev where the probe landed is not good enough to get an artifact that includes the probe.

@jlast use the latest m-c sha1, should work : e2bb11b88bd4

@jasonLaster
Copy link
Contributor Author

k

@jasonLaster jasonLaster merged commit e7aa85b into firefox-devtools:master Jan 24, 2018
jasonLaster added a commit to jasonLaster/debugger.html that referenced this pull request Jan 25, 2018
@jasonLaster
Add telemetry probes (firefox-devtools#4665)
5b0daa1
jasonLaster added a commit that referenced this pull request Jan 26, 2018
@jasonLaster
Add telemetry probes (#4665)
2a7a5c9
jasonLaster added a commit that referenced this pull request Jan 27, 2018
@jasonLaster
Add telemetry probes (#4665)
bbab553
jasonLaster added a commit that referenced this pull request Jan 27, 2018
@jasonLaster
Add telemetry probes (#4665)
44eeba1
jasonLaster added a commit that referenced this pull request Jan 29, 2018
@jasonLaster
Add telemetry probes (#4665)
d8120f7
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bwinton bwinton bwinton left review comments

@fmarier fmarier fmarier left review comments

@codehag codehag codehag left review comments

@juliandescottes juliandescottes juliandescottes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jasonLaster @codehag @juliandescottes @bwinton @fmarier