-
Notifications
You must be signed in to change notification settings - Fork 97
Add gRPC integration guide #355
Add gRPC integration guide #355
Conversation
Codecov Report
@@ Coverage Diff @@
## master #355 +/- ##
==========================================
- Coverage 95.28% 95.26% -0.03%
==========================================
Files 124 124
Lines 8339 8339
Branches 619 619
==========================================
- Hits 7946 7944 -2
- Misses 393 395 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #355 +/- ##
==========================================
- Coverage 95.28% 95.26% -0.03%
==========================================
Files 124 124
Lines 8339 8339
Branches 619 619
==========================================
- Hits 7946 7944 -2
- Misses 393 395 +2
Continue to review full report at Codecov.
|
examples/grpc/capitalize_client.js
Outdated
const { StackdriverTraceExporter } = | ||
require('@opencensus/exporter-stackdriver'); | ||
|
||
let tracer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional: what if you made setupOpencensusAndExporters
return the tracer so that you could assign it with const
. Maybe call it something like getSetupTracer
or something similar.
examples/grpc/capitalize_client.js
Outdated
function main () { | ||
const client = new rpcProto.Fetch('localhost:50051', | ||
grpc.credentials.createInsecure()); | ||
let data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional: could this just be const data = process.argv[2] || 'opencensus'
? Not sure if that's actually better but feels more compact.
}); | ||
|
||
setTimeout(() => { | ||
console.log('done.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the role of this message / is there a better way to check for doneness than waiting one minute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The minimum reporting period for Stackdriver is 1 minute. The thread with the StackdriverStatsExporter
must live for at least the interval past any metrics that must be collected, or some risk being lost if they are recorded after the last export. We planned to add a flush
method instead of doing this.
I will enable StackdriverStatsExporter
here, once the gRPC stats are available #270
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, interesting. Can you add a comment to explain this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
// for more details. | ||
// Expects ADCs to be provided through the environment as ${GOOGLE_APPLICATION_CREDENTIALS} | ||
// A Stackdriver workspace is required and provided through the environment as ${GOOGLE_PROJECT_ID} | ||
const projectId = process.env.GOOGLE_PROJECT_ID; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worth specifying above that this is intended to run on the Google Cloud Platform? Or is that already implied somewhere?
examples/grpc/capitalize_server.js
Outdated
const { StackdriverTraceExporter } = | ||
require('@opencensus/exporter-stackdriver'); | ||
|
||
let tracer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar optional comment here on assigning via const
with the return value of the setup function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
examples/grpc/README.md
Outdated
@@ -2,7 +2,7 @@ | |||
|
|||
Our service takes in a payload containing bytes and capitalizes them. | |||
|
|||
Using OpenCensus Node, we can collect traces of our system and export them to the backend of our choice, to give observability to our distributed systems. | |||
Using OpenCensus Node, we can collect traces of our system and export them to the backend of our choice(we are using Stackdriver for this example), to give observability to our distributed systems. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: add space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Node version of -> https://opencensus.io/guides/grpc/go/