Added ignoreUrls support

[whs]

This add ignoreUrls support by adding a check to CoreTracer.

Ideally, I would add it to the HTTP tracer directly but that would 1) requires interface change to pass the configuration to instrumenters and 2) it would requires modifying all tracers to honer the flag.

I'm not sure what's the intention of the interface's designer here, so I decided to go with the path of least resistance.

[justindsmith]

Hey @whs thanks for the first PR! :)

Looking a bit further, I’m not sure including the ignoreUrls in the tracing config is actually the right approach. As you pointed out, it adds too much plugin-specific logic into the root tracing config, which isn’t a good pattern.

You mention that the ideal would be to add it to the http plugin directly, and I think we can actually do that with minimal effects and no interface breaking.

I’d suggest we do the following:

1. Update the opencensus-instrumentation-http package to accept a configuration object that includes the ignoreUrls pattern and enforces the ignore logic. The default should be no ignoring (so there is no change in default behavior).
2. In your client code, you can instantiate the http plugin with the ignoreUrls configuration value you need and pass that through the plugins tracing configuration, which should override the default http plugin and use your instance instead. (Note that this might get a little weird as you actually need to create a file that can be required that exports a plugin instance object for your configuration that the plugin-loader will load. We might want to improve this if instantiating plugins becomes a more common workflow.)
3. We should remove the ignoreUrls configuration from the tracing to remove confusion (since it wasn’t being used before anyways as you point out).

Generally, I think it's better to keep instrumentation-specific configurations localized to the instrumentation library as much as possible. Things will only get more confusing when we add more instrumentation.

What do you think about that? If you need help with anything don’t hesitate to ask.

@fabiogomessilva Adding you just in case you had different intentions for this config option.

/cc @isaikevych

[whs]

I just got back from re:invent to start working on this.

I'm not sure how to pass a configuration object to the instrumenter.

• Looking at the entrypoint of instrumenter: plugin-loader is calling instrumenter.enable(moduleToPatch, tracer, version, basedir)
• So the only place we could get any configuration passed is by reading a field off tracer.
• The Tracing interface has no field that expose config passed to start()
  • I could add one but that means all tracers would need to be updated. It's also an interface not abstract class so future implementors would need to duplicate this functionality.

At this point I'd suggest these changes:

• Change TracingConfig.plugins to string: string|PluginNameAndOption, similar to webpack loader configuration
• Change BasePlugin.enable to accept one more argument: the option.

Should be mostly compatible with existing code.

[justindsmith]

Yeah, my thought was to have the user write a new module that would pass the config to the plugin implementation, but I like your idea much better. It's also backward compatible, which is great. (As long as the plugin can handle an undefined configuration by default.)

I think that's the way to go, and having a way to send configuration to the plugins would be a great feature!

Would you have time to implement this? Or would you need some help on some of it?

[whs]

Done! New version should be working even better than the original one.

[justindsmith]

Awesome! Thanks @whs! :)

I have a couple code comments to look at.

Also, the https and http2 plugins extend from http, so we should verify that they also work with the new config options for the http module. I'm a little worried they won't, but we should update those tests to include the same checks you added in the test-http.ts and we'll know for sure. I don't think it would be much more work to move the same logic from http into https and http2.

[whs]

I'm having problem with http2. It seems that the original URL is not recorded in the session object, yet our interface requires the full URI for outgoing requests.

I'm thinking of leaving http2 unsupported for now.

[justindsmith]

I'm having problem with http2. It seems that the original URL is not recorded in the session object, yet our interface requires the full URI for outgoing requests.

I'm thinking of leaving http2 unsupported for now.

I think that's fine for now as long as we document it. This is more of an advanced feature, so having some corner-cases is going to be expected.

[justindsmith]

@whs I think the failing tests are just code style problems. You should be able to run npm run fix in any of the packages that are failing and it will fix the code style problems for you.

[codecov-io]

Codecov Report

Merging #160 into master will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #160      +/-   ##
==========================================
+ Coverage   94.57%   94.61%   +0.03%     
==========================================
  Files         101      101              
  Lines        7242     7311      +69     
  Branches      681      690       +9     
==========================================
+ Hits         6849     6917      +68     
- Misses        393      394       +1

Impacted Files	Coverage Δ
src/trace/instrumentation/base-plugin.ts	7.69% <0%> (-0.21%)	⬇️
src/trace/model/tracer.ts	84.46% <0%> (ø)	⬆️
test/test-https.ts	99.46% <0%> (+0.07%)	⬆️
test/test-http.ts	99.43% <0%> (+0.07%)	⬆️
src/http.ts	89.8% <0%> (+1.47%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93d0436...f80e4dc. Read the comment docs.

[whs]

Merged master and test passed for Stackdriver. Anything else blocking the merge?

[justindsmith]

Thanks @whs! This looks good. One last thing... can you update the CHANGELOG.md file and add a new bullet under the Unreleased section? That should be the last thing and then I can merge this in!

You can use the following:

• Add ignoreUrls support to the http and https tracing instrumentations.

[whs]

Added!

[justindsmith]

Looks good! Thanks @whs for adding this!