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

Quickstart/Node.js Metrics: Add Node.js Metrics Codelab#513

Open
PikBot wants to merge 1 commit intocensus-instrumentation:masterfrom
PikBot:nodejs-codelabs
Open

Quickstart/Node.js Metrics: Add Node.js Metrics Codelab#513
PikBot wants to merge 1 commit intocensus-instrumentation:masterfrom
PikBot:nodejs-codelabs

Conversation

@PikBot
Copy link
Copy Markdown
Contributor

@PikBot PikBot commented Dec 24, 2018

Codelab for Node.js metrics quickstart

@odeke-em please review!

Comment thread codelabs/nodemetrics.md
AggregationType.COUNT,
[methodTagKey],
"The number of lines from standard input"
)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Semicolon (;) is missing.

Comment thread codelabs/nodemetrics.md
// Bucket Boudaries:
// [>=0B, >=5B, >=10B, >=15B, >=20B, >=40B, >=60B, >=80, >=100B, >=200B, >=400, >=600, >=800, >=1000]
[0, 5, 10, 15, 20, 40, 60, 80, 100, 200, 400, 600, 800, 1000]
)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here, Semicolon (;) is missing.

Comment thread codelabs/nodemetrics.md
const stats = new Stats();

// Add your project id to the Stackdriver options
const exporter = new StackdriverStatsExporter({projectId: "your-project-id"});
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it is worth to mention below lines here,

// Exporters use Application Default Credentials to authenticate.
// See https://developers.google.com/identity/protocols/application-default-credentials
// for more details.

WDYT?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, that would be useful for the reader.

Comment thread codelabs/nodemetrics.md
stats.record({
measure: mLatencyMs,
tags,
value: (new Date()) - startTime.getTime()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be value: new Date().getTime() - startTime.getTime() . Also, startTime variable is not defined anywhere in the code.

@mayurkale22
Copy link
Copy Markdown
Member

Following suit with #512, we should use open source exporter (Prometheus) for Node.js codelabs.

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.

4 participants