Node.js: Update examples #579

mayurkale22 · 2019-02-13T01:06:45Z

This PR contains the following changes.

Node.js 0.0.9 version has introduced butch of changes for Stats (Metrics) examples including TagKey, TagValue and TagMap.
Change var -> const.
Fixes quickstart/node.js: use open source exporters for tutorials #355 -> use the open source exporter (Prometheus) for quickstart tutorials.

odeke-em · 2019-02-13T01:08:28Z

content/exporters/custom-exporter/Node.js/Metrics.md

+  [myTagKey], 
+  'my view'
+);
+globalStats..registerView(view);


This doesn't look right globalStats.., double dots "..". Were you able to run this code?

Yes, I ran the example and it works as expected.

odeke-em · 2019-02-13T01:09:24Z

content/exporters/custom-exporter/Node.js/Metrics.md

 // And register it
-stats.registerExporter(exporter);
+globalStats.registerExporter(exporter);


Why are we changing this to globalStats? What's up with stats? Other languages use the same or is another stats used somewhere?

Earlier we used to create Stats instance (new Stats()) and used in the application. With new release, we have changed it to a global singleton Stats instance (globalStats). This behavioral change is introduced to match other language implementation. (The name globalStats is specific to Node library).

odeke-em · 2019-02-13T01:10:01Z

content/exporters/custom-exporter/Node.js/Trace.md


 // Let's create an instance of our just created exporter
-var exporter = new MyConsoleTraceExporter();
+const exporter = new MyConsoleTraceExporter();
 // And start tracing with it
 tracing.registerExporter(exporter).start();

 // Now, lets create a simple HTTP 2 server


s/HTTP 2/HTTP/2/g

odeke-em · 2019-02-13T01:11:12Z

content/exporters/supported-exporters/Node.js/Instana.md

-
-var exporter = new instana.InstanaTraceExporter();
+const tracing = require('@opencensus/nodejs');
+const { InstanaTraceExporter }= require('@opencensus/exporter-instana');


This seems spurious, perhaps just keep the const change and then as it was before i.e.

const instana

I personally think the destructuring import is fine as that is a fairly common pattern in JS and then it saves you referencing it via the namespace below (i.e. you don't need to say const exporter = new instana.InstanaTraceExporter(); you can just say const exporter = new InstanaTraceExporter();)

odeke-em · 2019-02-13T01:13:11Z

content/exporters/supported-exporters/Node.js/Jaeger.md

-var core = require('@opencensus/core');
-var tracing = require('@opencensus/nodejs');
-var jaeger = require('@opencensus/exporter-jaeger');
+const { logger } = require('@opencensus/core');


Perhaps let's make this style consistent: in some place we have const { objectName } = require(<import>) and in others const objectName = require(<import>) for example

const tracing = require('@opencensus/nodejs');

while here

const { logger } = require('@opencensus/core');

const { objectName } = require(<import>) i.e. destructuring import is preferred wherever it is feasible.

odeke-em · 2019-02-13T01:13:41Z

content/exporters/supported-exporters/Node.js/Prometheus.md

 const { PrometheusStatsExporter } = require('@opencensus/exporter-prometheus');

 // Add your port and startServer to the Prometheus options
 const exporter = new PrometheusStatsExporter({
  port: 9464,
-  startServer: false
+  startServer: true


What changed here from startServer: false to startServer: true?

So earlier, it was like two step approach

Don't start the Prometheus exporter server i.e. startServer: false

Explicitly call startServer method to start the Prometheus exporter server i.e.

exporter.startServer(function callback() { // Callback });

This can be possible by passing startServer: true option.

odeke-em · 2019-02-13T01:13:52Z

content/exporters/supported-exporters/Node.js/Prometheus.md

-});
+and then for our corresponding `prometheus.yaml` file:
+
+```shell


Good stuff including this!

odeke-em · 2019-02-13T01:14:15Z

content/exporters/supported-exporters/Node.js/Zipkin.md

-var tracing = require('@opencensus/nodejs');
-var zipkin = require('@opencensus/exporter-zipkin');
+const tracing = require('@opencensus/nodejs');
+const { ZipkinTraceExporter } = require('@opencensus/exporter-zipkin');


Mixing const tracing and const { ZipkinTraceExporter }

draffensperger · 2019-02-13T01:15:15Z

content/exporters/custom-exporter/Node.js/Metrics.md

+
+// Create and Register the view
+const view = globalStats.createView(
+  'my/view', 


Optional: what would you thin about adding comments for the name and description values e.g.

const view = globalStats.createView( /* name */ 'my/view', measure, AggregationType.LAST_VALUE, [myTagKey], /* description */ 'my view' );

draffensperger · 2019-02-13T01:17:25Z

content/exporters/supported-exporters/Node.js/Instana.md

-
-var exporter = new instana.InstanaTraceExporter();
+const tracing = require('@opencensus/nodejs');
+const { InstanaTraceExporter }= require('@opencensus/exporter-instana');


I personally think the destructuring import is fine as that is a fairly common pattern in JS and then it saves you referencing it via the namespace below (i.e. you don't need to say const exporter = new instana.InstanaTraceExporter(); you can just say const exporter = new InstanaTraceExporter();)

draffensperger · 2019-02-13T01:18:17Z

content/exporters/supported-exporters/Node.js/Prometheus.md

-});
+and then for our corresponding `prometheus.yaml` file:
+
+```shell


Should this be yaml instead of shell?

odeke-em · 2019-02-13T02:01:03Z

Thanks for working on this and for tagging me @mayurkale22! I'll humbly defer to @draffensperger since he has new Node.js expertise and is familiar with the new syntax than I am.

mayurkale22 · 2019-02-15T18:07:26Z

Is this good to merge?

odeke-em

LGTM, thank you @mayurkale22 for the work and @draffensperger for the reviews!

Node.js: Update examples

102a1cb

googlebot added the cla: yes label Feb 13, 2019

mayurkale22 requested review from odeke-em, draffensperger and songy23 February 13, 2019 01:06

odeke-em reviewed Feb 13, 2019

View reviewed changes

draffensperger reviewed Feb 13, 2019

View reviewed changes

shell->yaml, remove extra dot, HTTP 2->HTTP/2

cbe19fa

draffensperger approved these changes Feb 13, 2019

View reviewed changes

odeke-em approved these changes Feb 15, 2019

View reviewed changes

mayurkale22 merged commit 1690b8d into census-instrumentation:master Feb 15, 2019

mayurkale22 deleted the node_0.0.9 branch February 15, 2019 21:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node.js: Update examples #579

Node.js: Update examples #579

mayurkale22 commented Feb 13, 2019

odeke-em Feb 13, 2019

mayurkale22 Feb 13, 2019

odeke-em Feb 13, 2019

mayurkale22 Feb 13, 2019

odeke-em Feb 13, 2019

mayurkale22 Feb 13, 2019

odeke-em Feb 13, 2019

draffensperger Feb 13, 2019

odeke-em Feb 13, 2019

mayurkale22 Feb 13, 2019

odeke-em Feb 13, 2019

mayurkale22 Feb 13, 2019

odeke-em Feb 13, 2019

odeke-em Feb 13, 2019

draffensperger Feb 13, 2019

mayurkale22 Feb 13, 2019

draffensperger Feb 13, 2019

draffensperger Feb 13, 2019

mayurkale22 Feb 13, 2019

odeke-em commented Feb 13, 2019

mayurkale22 commented Feb 15, 2019

odeke-em left a comment

Node.js: Update examples #579

Node.js: Update examples #579

Conversation

mayurkale22 commented Feb 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odeke-em commented Feb 13, 2019

mayurkale22 commented Feb 15, 2019

odeke-em left a comment

Choose a reason for hiding this comment