This repository has been archived by the owner on Oct 3, 2023. It is now read-only.
Proof of Design Implementation of a first design proposal for opencensus-nodejs #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Op 30 See merge request opencensus/opencensus-node!1
…ving class files Span.ts, Trace.ts and Tracer.ts to trace/model
…ving class files Span.ts, Trace.ts and Tracer.ts to trace/model
…node into OP-31.v2
Merge 'Op 31.v2' into 'dev' See merge request opencensus/opencensus-node!3
Add a README.md for the OpenCensus Node example See merge request opencensus/opencensus-node!8
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
|
I signed it! |
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
Adapted tests for dev See merge request opencensus/opencensus-node!9
|
I would like just to emphasize that code comments on classes and methods were not prioritized for this first v0 version. We know they are needed and important. They will be added along the way in the next sprints to come. |
|
I signed it! |
1 similar comment
|
I signed it! |
|
I signed it. |
|
I signed it! |
2 similar comments
|
I signed it! |
|
I signed it! |
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
kjin
reviewed
Apr 3, 2018
|
|
||
| import { Tracing } from './trace/tracing'; | ||
|
|
||
| module.exports = new Tracing() |
There was a problem hiding this comment.
Prefer export = new Tracing() to match usage of the import keyword.
| export class ConsoleLogExporter implements Exporter { | ||
|
|
||
| writeTrace(root: RootSpan) { | ||
| let rootStr: string = ( ` |
There was a problem hiding this comment.
It seems like the format of these log lines aren't consistent, can you normalize the padding and log messages here?
| let result:string[] = []; | ||
|
|
||
| result.push(rootStr) | ||
| result.push(`${spansStr.join("")}`) |
There was a problem hiding this comment.
Nit: join already returns a string.
| private translateSpan(span) { | ||
| return { | ||
| "name": span.name, | ||
| "kind": "SPAN_KIND_UNSPECIFIED", |
There was a problem hiding this comment.
kind should be specified here if possible.
| export class StackdriverOptions implements ExporterOptions { | ||
| projectId: string; | ||
|
|
||
| constructor(projectId: string) { |
There was a problem hiding this comment.
I don't think it's necessary to have a class for an options object.
| public writeTrace(root: RootSpan) { | ||
| // Builds span data | ||
| let spanList = [] | ||
| root.spans.forEach(span => { |
There was a problem hiding this comment.
Nit: Can use const spanList = root.spans.map(span => this.translate(span))
| } | ||
|
|
||
| // TODO: Rename to "flushBuffer" and "publish" | ||
| private sendTrace(projectId, authClient, resource) { |
There was a problem hiding this comment.
Nit -- it looks like this can be a static function.
Also, what is the justification for renaming to flushBuffer or publish? I think sendTrace is fine.
| }); | ||
|
|
||
| // write data to request body | ||
| let spansJson: string[] = spans.map((span)=> JSON.stringify(span)); |
There was a problem hiding this comment.
Suggestion: JSON.stringify the entire string rather than individual spans.
| return ns / 1e6 | ||
| } | ||
|
|
||
| public get hrtime() : [number, number] { |
| const EVENT_EMITTER_METHODS: Array<keyof EventEmitter> = | ||
| ['addListener', 'on', 'once', 'prependListener', 'prependOnceListener']; | ||
|
|
||
| class AsyncHooksNamespace implements CLSNamespace { |
There was a problem hiding this comment.
Something to keep in mind -- the Trace Agent's async_hooks implementation is going to change over time.
|
Hi Kelvin,
Thanks for your feedback so far.
For this first sprint, the classes in package src/exporters/* were more
like a proof of concept, we wanted to get a minimum code to export to
Stackdriver and Zipkin. So we are expecting to do a refactoring of this
code in the sprints to come.
Regards,
Fabio Silva.
2018-04-03 12:37 GMT-03:00 Kelvin Jin <[email protected]>:
… ***@***.**** commented on this pull request.
Still working through it... some initial comments.
------------------------------
In src/index.ts
<#1 (comment)>
:
> @@ -0,0 +1,5 @@
+'use strict'
+
+import { Tracing } from './trace/tracing';
+
+module.exports = new Tracing()
Prefer export = new Tracing() to match usage of the import keyword.
------------------------------
In src/exporters/exporter.ts
<#1 (comment)>
:
> +import { OnEndSpanEventListener } from '../trace/types/tracetypes'
+
+export interface Exporter extends OnEndSpanEventListener {
+ writeTrace(root: RootSpan);
+}
+
+export class NoopExporter implements Exporter {
+
+ writeTrace(root: RootSpan) {}
+ onEndSpan(root:RootSpan){}
+}
+
+export class ConsoleLogExporter implements Exporter {
+
+ writeTrace(root: RootSpan) {
+ let rootStr: string = ( `
It seems like the format of these log lines aren't consistent, can you
normalize the padding and log messages here?
------------------------------
In src/exporters/exporter.ts
<#1 (comment)>
:
> + onEndSpan(root:RootSpan){}
+}
+
+export class ConsoleLogExporter implements Exporter {
+
+ writeTrace(root: RootSpan) {
+ let rootStr: string = ( `
+ RootSpan: {writing RootSpan: traceId: ${root.traceId}, spanId: ${root.id}, name: ${root.name} }
+ `);
+ let spansStr: string[] =
+ root.spans.map((span)=>` ChildSpan: {traceId: ${span.traceId}, spanId: ${span.id}, name: ${span.name} }
+ `)
+ let result:string[] = [];
+
+ result.push(rootStr)
+ result.push(`${spansStr.join("")}`)
Nit: join already returns a string.
------------------------------
In src/exporters/stackdriver/stackdriver.ts
<#1 (comment)>
:
> + "traces": [
+ {
+ "projectId": this.projectId,
+ "traceId": root.traceId,
+ "spans": spanList
+ }
+ ]
+ }
+ //this.buffer[trace.traceId] = resource
+ this.authorize(this.sendTrace, resource);
+ }
+
+ private translateSpan(span) {
+ return {
+ "name": span.name,
+ "kind": "SPAN_KIND_UNSPECIFIED",
kind should be specified here if possible.
------------------------------
In src/exporters/stackdriver/options.ts
<#1 (comment)>
:
> + *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import {ExporterOptions} from "../exporterOptions"
+
+export class StackdriverOptions implements ExporterOptions {
+ projectId: string;
+
+ constructor(projectId: string) {
I don't think it's necessary to have a class for an options object.
------------------------------
In src/exporters/stackdriver/stackdriver.ts
<#1 (comment)>
:
> +const cloudTrace = google.cloudtrace('v1');
+
+export class Stackdriver implements Exporter {
+ projectId: string;
+ buffer: object;
+
+ constructor(options: StackdriverOptions) {
+ this.projectId = options.projectId;
+ this.buffer = {};
+ }
+
+ // TODO: Rename to "writeTrace"
+ public writeTrace(root: RootSpan) {
+ // Builds span data
+ let spanList = []
+ root.spans.forEach(span => {
Nit: Can use const spanList = root.spans.map(span => this.translate(span))
------------------------------
In src/exporters/stackdriver/stackdriver.ts
<#1 (comment)>
:
> + //this.buffer[trace.traceId] = resource
+ this.authorize(this.sendTrace, resource);
+ }
+
+ private translateSpan(span) {
+ return {
+ "name": span.name,
+ "kind": "SPAN_KIND_UNSPECIFIED",
+ "spanId": span.id,
+ "startTime": span.startTime,
+ "endTime": span.endTime
+ }
+ }
+
+ // TODO: Rename to "flushBuffer" and "publish"
+ private sendTrace(projectId, authClient, resource) {
Nit -- it looks like this can be a static function.
Also, what is the justification for renaming to flushBuffer or publish? I
think sendTrace is fine.
------------------------------
In src/exporters/zipkin/zipkin.ts
<#1 (comment)>
:
> + debug(`HEADERS: ${JSON.stringify(res.headers)}`);
+ res.setEncoding('utf8');
+ res.on('data', (chunk) => {
+ debug(`BODY: ${chunk}`);
+ });
+ res.on('end', () => {
+ debug('No more data in response.');
+ });
+ });
+
+ req.on('error', (e) => {
+ debug(`problem with request: ${e.message}`);
+ });
+
+ // write data to request body
+ let spansJson: string[] = spans.map((span)=> JSON.stringify(span));
Suggestion: JSON.stringify the entire string rather than individual spans.
------------------------------
In src/internal/clock.ts
<#1 (comment)>
:
> + public get duration(): number {
+ if (!this._ended){
+ return null
+ }
+ var ns = this.diff[0] * 1e9 + this.diff[1]
+ return ns / 1e6
+ }
+
+ public offset(timer: Clock): number {
+ var a = timer.hrtime
+ var b = this.hrtime
+ var ns = (b[0] - a[0]) * 1e9 + (b[1] - a[1])
+ return ns / 1e6
+ }
+
+ public get hrtime() : [number, number] {
This method is unused?
------------------------------
In src/internal/cls-ah.ts
<#1 (comment)>
:
> +
+import * as asyncHook from 'async_hooks'
+import {Context, Func, Namespace as CLSNamespace} from 'continuation-local-storage'
+import {EventEmitter} from 'events'
+import * as shimmer from 'shimmer'
+
+const wrappedSymbol = Symbol('context_wrapped');
+let contexts: {[asyncId: number]: Context;} = {};
+let current: Context = {};
+
+asyncHook.createHook({init, before, destroy}).enable();
+
+const EVENT_EMITTER_METHODS: Array<keyof EventEmitter> =
+ ['addListener', 'on', 'once', 'prependListener', 'prependOnceListener'];
+
+class AsyncHooksNamespace implements CLSNamespace {
Something to keep in mind -- the Trace Agent's async_hooks implementation
is going to change over time.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/Ajq6qtPNjpuzLCDT_omsSbrTNVHkvzOWks5tk5dAgaJpZM4TBBkr>
.
|
justindsmith
pushed a commit
to justindsmith/opencensus-node
that referenced
this pull request
Aug 15, 2018
The JaegerTraceExporterOptions accepts `tags` as a Tag array, but was processing those tags for the process information as if they were an object / Record instead of a Tag array. This was leading to the process key-value inside Jaeger to be the array index as key and the value as the key-value object. We now use the proper key-value information from the input to place into the process tags. There were two approaches we could have taken here: 1. To leave the input as a Tag array and process the input properly in the code or 2. To change the input to a key-value record / object (Record<string, TagValue), which would be handled properly by the existing code. We have chosen approach census-instrumentation#1 because a Jaeger process tag CAN support multiple tags with the same key name, which would not be supported in approach census-instrumentation#2. There is a chance that (because we were not previously exporting typescript types) users were passing in a Record format instead of Array, and thus this might be seen as a breaking change. A test has been added to validate the process information to ensure proper format. Also included are sticter typing for all thrift types, removing all `any` references and replacing with valid types.
kjin
pushed a commit
that referenced
this pull request
Aug 17, 2018
The JaegerTraceExporterOptions accepts `tags` as a Tag array, but was processing those tags for the process information as if they were an object / Record instead of a Tag array. This was leading to the process key-value inside Jaeger to be the array index as key and the value as the key-value object. We now use the proper key-value information from the input to place into the process tags. There were two approaches we could have taken here: 1. To leave the input as a Tag array and process the input properly in the code or 2. To change the input to a key-value record / object (Record<string, TagValue), which would be handled properly by the existing code. We have chosen approach #1 because a Jaeger process tag CAN support multiple tags with the same key name, which would not be supported in approach #2. There is a chance that (because we were not previously exporting typescript types) users were passing in a Record format instead of Array, and thus this might be seen as a breaking change. A test has been added to validate the process information to ensure proper format. Also included are sticter typing for all thrift types, removing all `any` references and replacing with valid types.
kjin
pushed a commit
to kjin/opencensus-node
that referenced
this pull request
Aug 24, 2018
…on#93) The JaegerTraceExporterOptions accepts `tags` as a Tag array, but was processing those tags for the process information as if they were an object / Record instead of a Tag array. This was leading to the process key-value inside Jaeger to be the array index as key and the value as the key-value object. We now use the proper key-value information from the input to place into the process tags. There were two approaches we could have taken here: 1. To leave the input as a Tag array and process the input properly in the code or 2. To change the input to a key-value record / object (Record<string, TagValue), which would be handled properly by the existing code. We have chosen approach census-instrumentation#1 because a Jaeger process tag CAN support multiple tags with the same key name, which would not be supported in approach census-instrumentation#2. There is a chance that (because we were not previously exporting typescript types) users were passing in a Record format instead of Array, and thus this might be seen as a breaking change. A test has been added to validate the process information to ensure proper format. Also included are sticter typing for all thrift types, removing all `any` references and replacing with valid types.
kjin
pushed a commit
to kjin/opencensus-node
that referenced
this pull request
Aug 24, 2018
…on#93) The JaegerTraceExporterOptions accepts `tags` as a Tag array, but was processing those tags for the process information as if they were an object / Record instead of a Tag array. This was leading to the process key-value inside Jaeger to be the array index as key and the value as the key-value object. We now use the proper key-value information from the input to place into the process tags. There were two approaches we could have taken here: 1. To leave the input as a Tag array and process the input properly in the code or 2. To change the input to a key-value record / object (Record<string, TagValue), which would be handled properly by the existing code. We have chosen approach census-instrumentation#1 because a Jaeger process tag CAN support multiple tags with the same key name, which would not be supported in approach census-instrumentation#2. There is a chance that (because we were not previously exporting typescript types) users were passing in a Record format instead of Array, and thus this might be seen as a breaking change. A test has been added to validate the process information to ensure proper format. Also included are sticter typing for all thrift types, removing all `any` references and replacing with valid types.
kjin
pushed a commit
to kjin/opencensus-node
that referenced
this pull request
Aug 24, 2018
…on#93) The JaegerTraceExporterOptions accepts `tags` as a Tag array, but was processing those tags for the process information as if they were an object / Record instead of a Tag array. This was leading to the process key-value inside Jaeger to be the array index as key and the value as the key-value object. We now use the proper key-value information from the input to place into the process tags. There were two approaches we could have taken here: 1. To leave the input as a Tag array and process the input properly in the code or 2. To change the input to a key-value record / object (Record<string, TagValue), which would be handled properly by the existing code. We have chosen approach census-instrumentation#1 because a Jaeger process tag CAN support multiple tags with the same key name, which would not be supported in approach census-instrumentation#2. There is a chance that (because we were not previously exporting typescript types) users were passing in a Record format instead of Array, and thus this might be seen as a breaking change. A test has been added to validate the process information to ensure proper format. Also included are sticter typing for all thrift types, removing all `any` references and replacing with valid types.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tracing class
Tracer class
Plugin Loader class
http plugin
mongodb-core plugin
Simple exporters to zipkin and stackdriver - no buffer
Allways sample strategy