Exporter/Stackdriver: Use Trace SDKs (v2 API) #338
Conversation
| droppedMessageEventsCount: number): types.TimeEvents { | ||
| const timeEvents: types.TimeEvent[] = []; | ||
| if (annotationTimedEvents) { | ||
| annotationTimedEvents.forEach(annotation => { |
There was a problem hiding this comment.
Could this be a map rather than forEach + push?
| boolean): types.AttributeValue { | ||
| switch (typeof value) { | ||
| case 'number': | ||
| return {intValue: String(value)}; |
There was a problem hiding this comment.
Could you make this be doubleValue? The number type in JS is a double so we would be losing accuracy for floating point values.
I would think if something is a BigInt and fits in 64 bits it should be made to be an intValue, which it looks like Node 10 supports
There was a problem hiding this comment.
v2 API doesn't support doubleValue type for AttributeValue -> https://github.com/googleapis/google-api-nodejs-client/blob/master/src/apis/cloudtrace/v2.ts#L153-L166. Also intValue is considered as String type.
There was a problem hiding this comment.
Hmm, OK. Then here are a couple options:
- Just always translate
numberto its string representation. This is probably my suggestion. Maybe add a TODO to change this once the API supports it. The OpenCensus trace proto hasdoubleValue. - Check whether the number fits in a 64-bit int (less than the max 64-bit int and no fractional part) and only then write it as an
intValueand otherwise useString.
I think intValue uses String type because number can't represent full accuracy 64-bit ints.
There was a problem hiding this comment.
Added TODO here to consider doubleValue type once available with cloudTrace V2 API. As per our offline discussion (+ your suggestion), will always translate number to its string representation for AttributeValue type.
| return new Promise((resolve, reject) => { | ||
| cloudTrace.projects.patchTraces(traces, (err: Error) => { | ||
| cloudTrace.projects.traces.batchWrite(spans, (err: Error) => { |
There was a problem hiding this comment.
Any reason we are using HTTP and not gRPC? (If the answer is just "that's what we did before", could you create an issue to track moving to gRPC for this?)
There was a problem hiding this comment.
Good point, added TODO here.
| links: createLinks(span.links, span.droppedLinksCount), | ||
| status: {code: span.status.code}, | ||
| sameProcessAsParentSpan: !span.remoteParent, | ||
| childSpanCount: null, |
mayurkale22
force-pushed
the
stackdriver_v2
branch
from
f1f44dd
to
4881ff7
Compare
Codecov Report
@@ Coverage Diff @@
## master #338 +/- ##
=========================================
+ Coverage 94.98% 95.18% +0.2%
=========================================
Files 120 122 +2
Lines 8332 8472 +140
Branches 741 750 +9
=========================================
+ Hits 7914 8064 +150
+ Misses 418 408 -10
Continue to review full report at Codecov.
|
Fixes #322 and #39