Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tracer implementation for the browser #276

Closed
bg451 opened this issue Sep 17, 2019 · 7 comments · Fixed by #334
Closed

Add tracer implementation for the browser #276

bg451 opened this issue Sep 17, 2019 · 7 comments · Fixed by #334
Assignees
Milestone

Comments

@bg451
Copy link
Member

bg451 commented Sep 17, 2019

The goal here is to create an initial pass at a package that contains a tracer implementation that works in the browser. This means creating a new package called opentelemetry-tracer-web which pulls in the basic tracer along with any platform specific code, scripts to compile and minify the package, and any appropriate initial testing. We use CircleCI for automated testing, which I believe allows for headless chrome testing.

cc @draffensperger is there anything else you'd add to this for an initial go at the web implementation?

@bg451
Copy link
Member Author

bg451 commented Sep 17, 2019

FYI @obecny is working on this, can't seem to add him as an assignee quite yet

@draffensperger
Copy link
Contributor

My understanding is that the basic tracer should have the small-scale platform specific code like calculating timestamps, etc. To the extent that it's currently Node-specific we should fix that and write Karma tests for it

Would you see opentelemetry-tracer-web as including any kind of automatic context propagation or collection of initial load spans? In OpenCensus Web we had separate packages for the initial load and for user interaction traces (context propagation with Zone.js).

@obecny
Copy link
Member

obecny commented Sep 17, 2019

Ok I have added platform specific for Span for performance.now(). The opentelemetry-tracer-web will use BasicTracer. Just looking at NodeTracer my assumption now is that I need to add appropriate Scope Manager partially based on AsyncHooksScopeManager.

@draffensperger
Copy link
Contributor

draffensperger commented Sep 17, 2019

I actually think the code for performance.now should be handled within the BasicTracer and even Core packages themselves. You can follow the same pattern we used for generating span IDs. See:

https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-core/src/platform/browser/id.ts

That gets swapped in when using webpack by this line:

"./src/platform/index.ts": "./src/platform/browser/index.ts"

The reason I think it's important to have that lightweight platform specific functionality in the Core and BasicTracer packages, is that then users can use those packages directly in the browser if they want to - they don't need to use the opentelemetry-tracer-web package, which may contain extra code. This is similar to the use case where some Node users may wish to use BasictTracer directly.

In other words, if we just need to make small changes to a package for utility functions, we just do that in the package using that. See also https://github.com/defunctzombie/package-browser-field-spec for details on the browser field in package.json that allows this approach to work.

WDYT?

@draffensperger
Copy link
Contributor

I think the way we would get an early win with this approach is to make a browser-specific example of using the basic-tracer package directly using a say, sample React app built with webpack or rollup. At that point we know the core SDK is browser compatible.

Then as a future action, we can make an opentelemetry-tracer-web that does fancier things around getting spans for the initial page load, XHRs, user interactions like clicks, etc. Those different functionality pieces could themselves be separate packages that opentelemetry-tracer-web composes (similar to how the Node tracer pulls in different package instrumentations)

@obecny
Copy link
Member

obecny commented Sep 18, 2019

Yes this is exactly where I'm adding this as you said (Span is part of BasicTracer and it was using performance.now()) :) the same way it is done with your mentioned example browser field in package.json. I have also added the karma for BasicTracer to simulate "browser". Once I finish the WebScopeManager - I think adding some real example that can be opened in browser is also essential for better understanding how it works - including myself :).

@obecny obecny mentioned this issue Sep 24, 2019
@mayurkale22 mayurkale22 added this to the Alpha v0.2 milestone Oct 3, 2019
@bg451
Copy link
Member Author

bg451 commented Oct 7, 2019

Woohoo! Awesome work @obecny!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants