allow specifying which auto-instrumented urls should have tracing headers

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 specifying which auto-instrumented urls should have tracing headers #237

Closed trescenzi closed 4 years ago

trescenzi commented 4 years ago

While the tracing headers are useful when sent to backends that expect, and utilize them, automatic instrumentation is also useful for just tracking the time and response status of requests to external services that aren't being tracked. If such an external service has a strict policy on allowed headers the automatically added headers will cause instrumented requests to error. So instead of not getting any information from those URLs this commit adds the ability to whitelist, and blacklist, the URLs that have tracing headers added to them.

The new options are built off of the existing options and are:

fetch_url_header_inclusion_patterns
fetch_url_header_exclusion_patterns
xhr_url_header_inclusion_patterns
xhr_url_header_exclusion_patterns

I didn't add any tests because there were not other tests that referenced these automatic instrumentation plugins. Let me know if you think I should add some. I also included the dist, not sure if that's correct or not.

trescenzi commented 4 years ago

@mwear would you like me to add tests for the general include/exclude url options as well? Not 100% sure what'll be involved there, gotta look at how tests are written right now, but I imagine they will look very similar and shouldn't be much more to add.

mwear commented 4 years ago

That would be awesome and a welcome addition @trescenzi!

trescenzi commented 4 years ago

I've added tests. Not certain this is what you were looking for though. I wasn't sure of a good way to fully test some of this without stubbing out, fetch and XMLHttpRequest as these are largely testing side effects of global functions. As a result I included mocks for XHR and Fetch into the browser suite. One I npm installed, fetch-mock, the other, FakeXMLHttpRequest, I copied the dist into test/browser as had been done for other libraries used in those tests. It would be easy to copy fetch-mock's dist as well but since it wasn't published to github I went with loading it through node_modules for now.

Let me know it this makes sense or if you had a different approach in mind.

andrewhsu commented 4 years ago

@obecny can you also have a review of the deps that are brought in?

trescenzi commented 4 years ago

Hey apologies for the delay here, been sidetracked by the world. Hope everyone's doing alright. I'm planning to get to this either tonight or tomorrow night.

trescenzi commented 4 years ago

I've updated this based on comments and gotten it up to date with master. Tried to make the mocking more clear and moved the header checks into shared function that are also hopefully a little more self documenting. Let me know if there's anything else that needs to be done.

mwear commented 4 years ago

Sorry for the delay @trescenzi . @obecny and I will review this on Monday.

mwear commented 4 years ago

@trescenzi I just wanted to check in on the status of this PR. Is it complete and in as good of a final state as it can be, aside from workarounds that are needed for testing?

trescenzi commented 4 years ago

@mwear I just pushed the changes that @obecny suggested for the _shouldTrace/_shouldAddHeaders methods. I separated out the _shouldTrace change in case you want to drop it because it's a refactor of existing code. Aside from that I believe it should be good to go.

mwear commented 4 years ago

@trescenzi This shipped yesterday in v0.30.0 and is available on NPM. Thanks again for all your hard work!