Allow empty access token and set default transport to proto

lightstep / lightstep-tracer-javascript

Lightstep distributed tracing library for Node.js and the browser

https://lightstep.com

MIT License

77 stars 66 forks source link

Allow empty access token and set default transport to proto #151

Closed austinlparker closed 5 years ago

austinlparker commented 5 years ago

Switch default transport to proto
Support empty access tokens
Fix proto reporter field to include correct attributes (platform type, platform version, command line, etc)
Fix browser proto route to not throw on response.
Update tests

austinlparker commented 5 years ago

Please note - this is a breaking change on merge as it changes the default transport to proto over http.

jmacd commented 5 years ago

What can we do about the breaking change?

austinlparker commented 5 years ago

Well, if someone's bringing in the package from NPM it's not a huge deal as minor bumps don't get automatically updated. I'm more concerned about people who may be using the dist files directly from github as an ersatz CDN.

austinlparker commented 5 years ago

Quick update - I went ahead and did some manual testing since I'm pretty sensitive to the transport change... Passed in a context through extract that resulted in this -

SpanContextImp {
  _baggage: {},
  _guid: 'ffffffffffffffff',
  _traceGUID: 'ffffffffffffffff' }

And was able to create a new span as a child of that context, then successfully serialized them to LightStep via protos -

So, I'm reasonably confident that we're not going to break anything with the proto change.

sbaum1994 commented 5 years ago

What's the behavior when it's empty access token and there's no local satellite running? Is there any helpful debug messages?

austinlparker commented 5 years ago

The satellite doesn't support sending an error message like that yet, so you'll get some generic response error from the transport.

austinlparker commented 5 years ago

FYI - this is also blocked on LS-7832.

sbaum1994 commented 5 years ago

Maybe a useful high verbosity info message? "INFO: You are running with an empty access token. This is only supported for Developer Mode reporting." or something?

austinlparker commented 5 years ago

sure, i like both of those ideas, i'll add them in.

sbaum1994 commented 5 years ago

sure, i like both of those ideas, i'll add them in.

TY, it LGTM on a cursory review