[rails] Add feature flags to Rails integration

[pawelchcki]

Put aspects of Rails integration behind feature flags, allowing finetuning of provided tracing data depending on specific user needs.

Key	Description	Default
flags	Flags used in rails instrumentation.	{ instrument_action_view_rendering: true, instrument_action_controller_processing: true, instrument_active_support_caching: true, use_rack_integration: true, use_active_record_integration: true}

[delner]

Thinking about the scenario where a user only wants to turn off a single feature, would that mean they then have to list every other feature except the one they don't want?

If that's the case, I'd suggest making the features option into a Hash, so a user could do something like c.use :rails, features: { instantiation: false }, for more concise usability.

Also I might suggest renaming features to flags because there might come a time where we want some generic option that isn't exactly a feature. Once we commit to adding this name to the API, we won't be able to change it easily. Just something to think about.

[pawelchcki]

Thanks for looking at this @delner.
Thats true, that when user want's to enable only a single feature it has to lists all other feature. This is something I thought could be addressed in separate PRs by introducing API's like disabled_feature and maybe even enabled_features.

For now since hashes are not merged in Options having the shorthand of { flag: false} would disable all other flags unless we would check it like so

enable if !configuration[:flags].has_key?(flag) || configuration[:flags][flag]

[pawelchcki]

Ok I've extended the setter functionality a bit, to allow setters that can merge values with default. It should now allow for shorter way to set and disable the flags

Instead of extending setter, I've opted for option that doesn't require extending APIs

[pawelchcki]

Since we are introducing new API to allow more granularity when configuring integrations. Would you mind taking a second look @palazzem?

[delner]

These changes look fine to me. Let's ask @palazzem to see what he thinks about adding this kind of option to the API.

[palazzem]

@delner @pawelchcki I think it's great providing a fine-grain control on what integrations are enabled, considering we can keep reasonable defaults (so everything on by default). Two things I'm not sure are:

1. I think the naming is a bit weird because we have a lot of instrument_* that is a duplication. What if we call the field instrument (or similar) instead of flags and we remove the same prefix?
2. I'm saying (1) because I think the naming should be the same, so instead of use_active_record_integration, I would say instrument_active_record (the name could be changed after the first bullet point). I think it's better to enforce consistency around names so that it's easier to understand what's going on, and there is no difference from the users perspective if you're instrumenting active_record or using the active_record instrumentation.
3. I think rack integration must not be disabled in any case. It could lead to huge issues especially if other frameworks are used. Disabling rack while you have grape, will lead to missing traces. I think this is the only integration that must never be disabled.

[delner]

@palazzem I see, so options that are much more specific in name and purpose? I think that could make sense. Originally, I had suggested renaming it to flags in case we wanted to reuse this option as a general purpose "miscellaneous" options holder, but if we don't want or need something general purpose like that, I can see how instrument is better (because its more specific.)

[delner]

Just a thought, albeit requiring no change at all: this active_record line is mean to enable/disable "sub-integrations" that the Rails meta-integration activates.

For what it's worth, I had started on changes to address exactly this kind of configuration setting in my configuration overhaul, which ultimately was meant to produce a new configuration API that resembles this prototype. The purpose of this is eerily similar to that.

[delner]

@palazzem @pawelchcki Just wanted to get your opinion on this: how would you feel about bumping this to 0.13.0 (next week's release)? Or 0.14.0?

My suggestion for doing so is that 1) this isn't really a bug fix in itself as it is a new feature, and 2) it would grant us an opportunity to reconcile the API changes in this PR with my already-written API changes suggested in this Gist and implemented in this branch. By reconciling these, we could avoid mutating the API too frequently and isolate the API changes to a single release (0.12.x --> 0.13.0)

[palazzem]

@delner agree. Let's bump the change to 0.13 if it's not required to address a specific issue.

[delner]

Okay. BTW, the aforementioned changes can be found in #450 and #451 .

@pawelchcki Could you check those out, tell me what you think? They don't yet implement this specific change, but it would be easy to implement (see my sample code in #450 for an idea of what the API might look like). If you like what you see there, then we could rebase these changes on #451 and make it a part of that.