Bring up to latest OT spec (and more) #79
Conversation
There was a ref to some deadcode; removed with no ill effects.
Tests are almost assuredly broken. I've removed a bunch of deprecated behavior along the way.
|
|
||
| queryUserInfo(span, username, function(err, user) { | ||
| if (err) { | ||
| span.imp().exception('Error in queryUserInfo', err); | ||
| span.exception('Error in queryUserInfo', err); |
There was a problem hiding this comment.
Remove this and make it a standard log call instead?
| @@ -135,6 +138,7 @@ function queryUserInfo(parentSpan, username, callback) { | |||
| }); | |||
|
|
|||
| // In parallel, query the recent events activity for the user | |||
| console.log(json.received_events_url); | |||
There was a problem hiding this comment.
Just checking: is this intentional or a debug statement that didn't get removed?
There was a problem hiding this comment.
debug statement, thanks -- removing.
| @@ -51,7 +51,7 @@ | |||
| "istanbul": "^0.4.4", | |||
| "json-loader": "^0.5.4", | |||
| "mocha": "^2.3.4", | |||
| "opentracing": "^0.11.0", | |||
| "opentracing": "^0.13.0", | |||
There was a problem hiding this comment.
I'd suggest locking this to a specific version: i.e., remove the caret and make it "opentracing": "0.13.0".
| } | ||
| fields.push(new crouton_thrift.KeyValue({ | ||
| Key : keyStr, | ||
| Value : valStr, |
There was a problem hiding this comment.
Is null a valid value for Value? Or should we skip the push entirely? I don't recall off the top of my head how Thrift and the collector process a value of null.
There was a problem hiding this comment.
at line 48 above we early-exit from the _each... i.e., we drop.
|
@bcronin please take a look when you get a chance. I found a few minor issues with more careful testing. This could be polished further but IMO it's worth merging this to have a version of the LightStep lib that's up-to-date OT-wise. |
| <script src="http://docs.lightstep.com/dist/opentracing-javascript/current/opentracing-browser.min.js"></script> | ||
| <script src="http://docs.lightstep.com/dist/lightstep-tracer-javascript/current/lightstep-tracer.min.js" | ||
| <script src=".../opentracing-browser.min.js"></script> | ||
| <script src=".../lightstep-tracer.min.js" |
There was a problem hiding this comment.
The ... threw me off as it wasn't obviously that wasn't literally what should be there. I might suggest simply leaving it as <script src="opentracing-browser.min.js"></script> and leaving it to the reader to set the URL to the context-specific URL.
There was a problem hiding this comment.
makes sense. I have done
<script src="PATH/TO/opentracing-browser.min.js"></script>
<script src="PATH/TO/lightstep-tracer.min.js"
This is a big PR. The only sane way to review will be by commit, though even that is difficult. I'm happy to go through it interactively.
This change is needed for two main reasons:
I introduced some not-strictly-necessary breaking changes since we essentially had to in order to be OT-friendly. These were intended to make library init more conventional (as much as I could find a convention in other OSS libs) and to remove some LightStep-specific extensions in certain places.
There is still work to be done here... biggest items while they're still in my cache:
tracer_imp.jsinto smaller piecesThere is also internal documentation (outside of this repo) that will need to be updated.