[tags] service instance tag should not be excluded if specified.

[truthbk]

This PR aims to allow the exclusion of the service jmx attribute that would otherwise be assigned as a tag while keeping the tag for the service as specified on the JMXFetch config. For instance, with this example configuration:

init_config:

instances:
    -   process_name_regex: .*surefire.*
        name: jmx_test_service_override_instance
        service: test       # <------ WILL BE INCLUDED
        tags:
            env: stage
            newTag: test
        conf:
            - include:
               domain: org.datadog.jmxfetch.test
               exclude_tags:
                    - env
                    - type
                    - newTag
                    - service       # <------ WILL BE EXCLUDED
               attribute:
                    ShouldBe1000:
                        metric_type: gauge
                        alias: test1.gauge
                    Int424242:
                        metric_type: histogram
                        alias: test1.histogram

[olivielpeau]

Taking a step back, I think there are 2 ways to solve this use case:

1. make the exclude_tags list apply only to the tags that are inferred from the bean parameters, and not to the tags that come from the instance-level or include-level tags config key. I can't think of a use case where excluding a tags-configured tag with exclude_tags is useful, since these tags don't add cardinality to the collected metrics (unless users want to remove the jmx_instance tag for example so that the Agent's dogstatsd server does aggregation across JMXFetch instances, but I don't know if this is a realistic use case).
   However, the current implementation and tests make exclude_tags explicitly exclude instance/include-level configured tags as well, so trying to change this now would require carefully considering all possible use cases. Or introducing a new config option such as exclude_bean_tags.
2. we can decide to limit this change to only the service config key, which is close to what you implemented so far in this PR. In this case, for consistency, I think exclude_tags values should still apply to a service: tag defined under the tags config key. Then, to keep the implementation simpler, instead of keeping a reference to the whole instanceTags in JMXAttribute and then matching the service tag explicitly, I think it'd be better to make the Instance pass the service value as a separate variable to JMXAttribute (and not pass it in the instanceTags map), and then simply make JMXAttribute add this service value as the service: tag after the exclude_tags logic.

Overall, I think option 2. is better for now (with the changes I suggested), WDYT?

A new config option named exclude_bean_tags could make sense in the future, to implement option 1.

[truthbk]

@olivielpeau I'm not entirely sure why doing (2) changing the api to pass a string for the service is much better than the proposed approach. The overhead shouldn't be that great because it's just a reference to a map, so we're not creating a Map per JmxAttribute, there should just be a (typically small) map per config instance. Passing on the map enables that if we ever need to do this for something other than service we could do it easily. The hashing could be a factor, but since the attributes are cached it shouldn't be too bad. Let me know what you think.

[olivielpeau]

I think (2) has 2 benefits over the current implementation:

1. no need to keep the full instanceTags map as an attribute: makes the JMXAttribute keep less state, which I think is a benefit (purely in terms of having less state and less room for side effects, not in terms of performance I agree). If we want to do more in the future with the full list of instanceTags, we could add it then, but until then I think it's better to keep things as simple as possible.
2. it makes the logic to add the service: tag from the actual service much simpler.

The rest of the logic in this class is complex enough (this is legacy code for sure), so I'm trying to avoid adding any unnecessary complexity.

[truthbk]

If you feel strongly about it, I'll make the change, but I need to change 4 interfaces to the various JmxAttribute sub-classes to pass in the service attribute on something that is already available in the provided parameters to the base-class.

[olivielpeau]

A couple more comments, let me know

[olivielpeau]

LGTM, thanks!