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

Example : Web Client Monitoring #218

Merged
merged 17 commits into from
Dec 11, 2018
Merged

Example : Web Client Monitoring #218

merged 17 commits into from
Dec 11, 2018

Conversation

alexamies
Copy link
Contributor

Example of using OpenCensus to instrument browser code

@mayurkale22 mayurkale22 self-requested a review December 4, 2018 19:35
@mayurkale22 mayurkale22 changed the title Initial checkin Example : Web Client Monitoring Dec 4, 2018
examples/stats/web_client_monitoring/web_client/.gitignore Outdated Show resolved Hide resolved
examples/stats/web_client_monitoring/package.json Outdated Show resolved Hide resolved
examples/stats/web_client_monitoring/app.js Outdated Show resolved Hide resolved
examples/stats/web_client_monitoring/app.js Outdated Show resolved Hide resolved
examples/stats/web_client_monitoring/README.md Outdated Show resolved Hide resolved
examples/stats/web_client_monitoring/README.md Outdated Show resolved Hide resolved
@bogdandrutu
Copy link
Contributor

Consider to use this repo https://github.com/census-instrumentation/opencensus-web

Copy link
Member

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

This is awesome @alexamies, thank you!

I've added some comments about just using mLatencyMs with various tags instead of separate measures, otherwise looks good!

examples/stats/web_client_monitoring/app.js Outdated Show resolved Hide resolved
examples/stats/web_client_monitoring/app.js Outdated Show resolved Hide resolved
examples/stats/web_client_monitoring/app.js Outdated Show resolved Hide resolved
Copy link
Member

@mayurkale22 mayurkale22 left a comment

Choose a reason for hiding this comment

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

LGTM, added one minor nit.

examples/stats/web_client_monitoring/README.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@alexamies alexamies left a comment

Choose a reason for hiding this comment

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

Re Bogdan's comment above about

Consider to use this repo https://github.com/census-instrumentation/opencensus-web

As discussed offline, this PR is better living in opencensus-node that opencensus-web because it is demonstrating use of the Node API.

examples/stats/web_client_monitoring/README.md Outdated Show resolved Hide resolved
@mayurkale22
Copy link
Member

@odeke-em @justindsmith If no other comments, I will merge this PR later today.

Copy link
Member

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

Thank you for working on this @alexamies, LGTM!

const valueDNSLookup = "dns_lookup";
const valueLoad = "load";
const valueWeb = "web";
let tags = { phase: valueDNSLookup, client: valueWeb };
Copy link
Member

Choose a reason for hiding this comment

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

For later updates after this merge, we don't need to declare this upfront and can instead
do it inline while recording e.g.

stats.record({
   measure: mLatencyMs,
   { phase: valueDNSLookup, client: valueWeb },
   value: dnsTime
});

ditto for the other stats.record calls but that can wait after this first pass.

@mayurkale22
Copy link
Member

Thank you for working on this @alexamies

@mayurkale22 mayurkale22 merged commit 6391818 into census-instrumentation:master Dec 11, 2018
@alexamies alexamies deleted the web_client_monitoring branch December 16, 2018 16:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants