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

Wildcard support on domains #57

Merged
merged 1 commit into from
Jul 13, 2015
Merged

Conversation

olivielpeau
Copy link
Member

@yannmh
Adds support for wildcards at the end of domain names.

For instance:

- include:
   domain: org.foo*

would match the domains org.foo and org.foo.bar, but not org.foobar.

I'll update https://github.com/DataDog/documentation accordingly if you're ok with this behaviour.

@yannmh
Copy link
Member

yannmh commented May 26, 2015

Great work, thanks @olivielpeau.

I'll review your PR really soon 😃.

@yannmh yannmh modified the milestone: 5.5.0 May 26, 2015
@yannmh
Copy link
Member

yannmh commented May 26, 2015

Usage of wildcard is a bit confusing here: I'd expect org.foo* to match org.foobar too.

If we only want to add support for domain and subdomain, then maybe a simple include_subdomain option in the yaml file is enough. What do you think ?

@olivielpeau
Copy link
Member Author

Yeah I agree, the wildcard is confusing, the include_subdomain option is more intuitive.

@sjenriquez do you think adding an include_subdomain option is all the customer needs ?

@sjenriquez
Copy link

I believe include_subdomain works best for the customer based on his request:

Regarding your JMX / Java integration, is it possible to include all domains with a common prefix?

Based on the documentation, you have to configure it like
domain: my_domain

Is it possible to also use something like
domain: my_domain*

to include not only "my_domain" but also i.e. "my_domain.foo"?

@remh
Copy link

remh commented May 27, 2015

I think it makes more sense to use regexes for both domains and attributes, the whole thing being behind a feature flag.

It's way more flexible and will solve the issue one and for all.

@olivielpeau
Copy link
Member Author

We could allow patterns described in the ObjectName java doc here (basically allowing * and ? in the bean names/domains/attributes).

We would probably need some refactoring to implement these regexes: instead of selecting all the bean names and then filtering them ourselves to match them with the configuration, we could directly query the MBeanServer with our regexes (unless there's a good reason to use the current logic).

@olivielpeau
Copy link
Member Author

@remh If these patterns make sense to you I'll start implementing.

@remh
Copy link

remh commented May 28, 2015

I don't think we can query for particular objects in the JMX implementation of java 1.6 (it was i think introduced with Java 1.7), unfortunately java 1.6 is still used a lot and we need to support it.

So, we can indeed implement * and ? but we will need to convert this into proper regexes and then use regexes to match the bean names ourselves.

Let me know if you have more questions.

@olivielpeau
Copy link
Member Author

@remh Actually I was thinking about this specific method, and it seems to be part of java 6: http://docs.oracle.com/javase/6/docs/api/javax/management/MBeanServerConnection.html#queryMBeans(javax.management.ObjectName, javax.management.QueryExp)

That said we could indeed use proper regexes.

@remh
Copy link

remh commented May 28, 2015

Nice, however if we have to run 100 queries (to get 100 beans for example) it will likely be slower than iterating once over all the beans and do the matching ourselves.

Could you benchmark the two solution with different kind of configurations (most importantly the default ones used by the cassandra,tomcat, kafka and activemq integrations) ?

@yannmh
Copy link
Member

yannmh commented Jun 1, 2015

👍 for wildcard support.

However, I am not sure it meets the initial expectations: querying a domain and its subdomains, would require two 'include' statements i.e

- include:
   domain: org.foo
  ...
- include:
   domain: org.foo.*
  ...

@olivielpeau olivielpeau force-pushed the olivielpeau/domains-wildcard branch from 60d80ae to 1591df5 Compare June 11, 2015 18:35
@olivielpeau
Copy link
Member Author

@yannmh @remh
I've changed the regex support in a way that seemed the most natural to me: a regex can be used on the domain name and/or the bean name with the domain_regex and bean_regex keys.

For instance, a valid configuration would be:

conf:
    - include:
        domain_regex: org\.datadog.*
        bean_regex:
            - .*[,:]type=\w*someType.*
            - .*[,:]type=someOtherType.*

The bean and domain keys can still be used, their behaviour doesn't change.

The drawback is that we can't directly use regexes on specific keys like type or scope, the user would need to create a regex in bean_regex in order to match those keys.

@olivielpeau olivielpeau added this to the 5.5.0 milestone Jun 11, 2015
@yannmh
Copy link
Member

yannmh commented Jul 10, 2015

Looks great ! :shipit:

@olivielpeau
Copy link
Member Author

Merging then 🏁

olivielpeau added a commit that referenced this pull request Jul 13, 2015
@olivielpeau olivielpeau merged commit 3638eb9 into master Jul 13, 2015
@olivielpeau olivielpeau deleted the olivielpeau/domains-wildcard branch July 13, 2015 19:05
yannmh added a commit that referenced this pull request Jul 29, 2015
* [FEATURE] Wildcard support on domains and bean names. See [#57][]
* [IMPROVEMENT] Memory saving by limiting MBeans queries to certain
* scopes. See [#63][]

[#57]: #57
[#63]: #63
yannmh added a commit that referenced this pull request Jul 29, 2015
* [FEATURE] Wildcard support on domains and bean names. See [#57][]
* [IMPROVEMENT] Memory saving by limiting MBeans queries to certain
* scopes. See [#63][]

[#57]: #57
[#63]: #63
[skip ci]
@yannmh yannmh mentioned this pull request Jul 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants