rochdev / datadog-tracer-js

[DEPRECATED] OpenTracing tracer implementation for Datadog in JavaScript.
MIT License
39 stars 8 forks source link

Browser updates? #37

Closed randy-girard closed 6 years ago

randy-girard commented 7 years ago

Hello. I am curious if there are any updates on the browser side of this, or a sample of how this works?

Thanks!

rochdev commented 7 years ago

The feature is still not complete but it shouldn't be too hard to do. I have already separated the code for node and the browser, so all that is left should be to test it. There is also a bit of cleanup to be done but I can do that as a refactor later.

I'll keep you posted when it's done, hopefully by next week.

tpiecora commented 6 years ago

@rochdev is this ready to go for browser?

rochdev commented 6 years ago

Not yet as it turned out to be more complicated than I anticipated to get it right and I haven't had much time to work on it.

It seems a lot more people than I thought want this feature, so I'll try to release something as soon as I can and keep optimizations for a future upgrade.

Sorry for the delay!

matthopson commented 6 years ago

@rochdev For what it’s worth, I spent some time on your branch fixing up a few things, but I imagine I ended up stuck about where you are - needing to actually send traces off requires some sort of auth for the client app and then a server with the exposed API that can take the request.

At least that’s the way I understood it. I made some asumptions on some things, but I’m happy to at least share what I’ve done in case it’s helpful on this.

rochdev commented 6 years ago

@matthopson I would definitely be interested to see the changes you've made.

For auth I would like to provide a way to configure it without exposing implementation details while being compliant with the OpenTracing API. Maybe some kind of built-in interceptors for the most common use cases? For example getting an access token from local storage, a cookie or a callback and automatically sending it in the Authorization header. Of course, some kind of gateway would need to be implemented on the server as well.

What do you think?

sadir commented 6 years ago

Hi @rochdev! First off thanks for maintaining this library. How is browser support going? We are looking to add in browser dd tracing to our node application as well as server side tracing and would be keen on this functionality.

rochdev commented 6 years ago

@sadir Unfortunately I haven't had much time to work on it. If I can release a first version it would be without authentication support (other than a cookie for a traditional web app). Would that be enough for your use case?

sadir commented 6 years ago

@rochdev yep that should do 😄 thanks. Any ideas on when you could do it?

rochdev commented 6 years ago

@sadir I'm doing a bit of testing right now of a manually tested unoptimized version and it's going well so far. I'll try to have something our by tomorrow and keep you posted either way.

rochdev commented 6 years ago

Oh well I couldn't resist making an optimized version 😄 Still on track for tomorrow

rochdev commented 6 years ago

Released in 0.4.0. IE9+ is supported plus all evergreen browsers.

sadir commented 6 years ago

@rochdev Thanks! Really appreciate the quick turnaround.

sadir commented 6 years ago

@rochdev Started implementing this in our react app today - running into an issue.

We already have this working in our express backend for the application, but I'd like to include it in the browser too. First I upgraded to v0.4:

> cat yarn.lock | grep datadog
datadog-tracer@^0.4.0:
  resolved "https://registry.yarnpkg.com/datadog-tracer/-/datadog-tracer-0.4.0.tgz#eee991f25af0a9e2e644a40fc503cf9b86c80715"

Next I added datadog-tracer to our react front end:

import DatadogTracer from 'datadog-tracer';

But then I get this error:

ERROR in ./~/datadog-tracer/src/propagation/state.proto.js
Module not found: Error: Can't resolve 'protobuf' in '/Users/morgansadr-hashemi/code/nested-tech/marketing-react/node_modules/datadog-tracer/src/propagation'
 @ ./~/datadog-tracer/src/propagation/state.proto.js 5:8-37
 @ ./~/datadog-tracer/src/propagation/binary.js
 @ ./~/datadog-tracer/src/tracer.js
 @ ./~/datadog-tracer/browser.js
 @ ./app/app.js
 @ multi eventsource-polyfill webpack-hot-middleware/client?reload=true ./app/app.js

Just in case I also imported protobufjs:

import 'protobufjs';
import DatadogTracer from 'datadog-tracer';

But it doesn't change the error.

Other stuff in case it helps:

> node -v
v6.9.1
> npm -v
5.5.1

Any pointers?

rochdev commented 6 years ago

@sadir It seems to be trying to use AMD to load the module instead of CommonJS for some reason. Can you create a minimal Webpack configuration that replicates the issue?

sadir commented 6 years ago

@rochdev I have created a repo which doesn't quite replicate the issue. I did get the same error when installing this package but a slightly different one when rendering the page. Have a read of the README for the steps I took to create the repo. If the repo isn't going to help let me know and I'll see what I can do.

rochdev commented 6 years ago

@sadir Fixed in v0.4.1. Thanks for the sample repo it helped a lot!