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

[jmxattribute] generate alias from regexp match #97

Merged
merged 2 commits into from
Apr 29, 2016
Merged

Conversation

yannmh
Copy link
Member

@yannmh yannmh commented Apr 27, 2016

Rebase of #94.

Additional changes

  • Refactor getAlias logic from JMXSimpleAttribute &
    JMXComplexAttribute to JMXAttribute
  • Allow group name substitutions in attribute/alias parameter.
    Available options are:
    • $attribute for the attribute name
    • $<parameter> for any of the bean parameters
  • Test coverage

Example:

bean: org.datadog.jmxfetch.test:foo=Bar,qux=Baz
attribute:
  toto:
    alias: my.metric.$foo.$attribute

returns a metric name my.metric.bar.toto.

Thanks again @alz !

LinkedHashMap<String, LinkedHashMap<String, String>> attribute = (LinkedHashMap<String, LinkedHashMap<String, String>>) (include.getAttribute());
if (attribute.get(fullAttributeName).get("alias_match") != null) {
Pattern p = Pattern.compile(attribute.get(fullAttributeName).get("alias_match"));
Matcher m = p.matcher(getBeanName().getCanonicalName() + "." + fullAttributeName);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alz I am wondering if we should not match on the bean names only, i.e. drop the the attribute name. What do you think ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking was that as the alias_match was defined inside the attribute yaml config it should include it in the match statement, and it also removed some extra duplicity where you would have to append ".#attribute#" to the end of the alias config line to reproduce the full metric name.

Of course using this method if you didn't want the same attribute name you'd have to add it to the alias_match regex, so it's a similar issue either way.

@yannmh yannmh force-pushed the yann/alias-regexp branch 2 times, most recently from 5e80d71 to 7681837 Compare April 28, 2016 14:30
@yannmh
Copy link
Member Author

yannmh commented Apr 28, 2016

Thanks for your feedback @alz.

On second thought, I think thealias_match parameter would be a little nebulous: from a user perspective, it's difficult to test and not clear to guess where the regular expression is applied (bean name versus attribute name for instance).

I went for another approach, which I think is easier to set up: allowing group name substitution directly into the attribute/alias parameter. The available options would be:

  • $attribute for the attribute name
  • $<parameter> for any of the bean parameters

For example, the following configuration

bean: org.datadog.jmxfetch.test:foo=Bar,qux=Baz
attribute:
  toto:
    alias: my.metric.$foo.$attribute

would return a metric name my.metric.bar.toto.

Can you think of any use case where this approach might be lacking ? Thanks again.

@alz
Copy link
Contributor

alz commented Apr 28, 2016

That seems to make sense. If I'm understanding right a bean named "domain:name=foo,bar=baz" would have $name and $bar mapped? Might also be good if we could map $domain too that way all bases are covered.

* Refactor `getAlias` logic from `JMXSimpleAttribute` &
  `JMXComplexAttribute` to `JMXAttribute`
* Allow group name substitutions in `attribute`/`alias` parameter.
  Available options are:
  * `$attribute` for the attribute name
  * `$domain` for the domain name
  * `$<parameter>` for any of the bean parameters
* Test coverage

Example:
```
bean: org.datadog.jmxfetch.test:foo=Bar,qux=Baz
attribute:
  toto:
    alias: my.metric.$foo.$attribute
```
returns a metric name `my.metric.bar.toto`.
@yannmh
Copy link
Member Author

yannmh commented Apr 29, 2016

That's right.

I added the $domain group name like you suggested. Let's merge it.

@yannmh yannmh merged commit 8719979 into master Apr 29, 2016
@yannmh yannmh deleted the yann/alias-regexp branch April 29, 2016 14:02
yannmh added a commit to DataDog/dd-agent that referenced this pull request May 13, 2016
**Changes**
* [BUGFIX] Report properly beans with ':' in the name. See [#90][],
* [#91][], [#95][] (Thanks [@Bluestix][])
* [BUGFIX] Sanitize metric names and tags, i.e. remove illegal
* characters. See [#89][]
* [BUGFIX] Support `javax.management.Attribute` attribute types. See
* [#92][] (Thanks [@nwillems][])
* [FEATURE] Add user tags to service checks. See [#96][]
* [FEATURE] Allow group name substitutions in attribute/alias
* parameters. See [#94][], [#97][] (Thanks [@alz][])

<!--- The following link definition list is generated by PimpMyChangelog
--->
[#89]: DataDog/jmxfetch#89
[#90]: DataDog/jmxfetch#90
[#91]: DataDog/jmxfetch#91
[#92]: DataDog/jmxfetch#92
[#94]: DataDog/jmxfetch#94
[#95]: DataDog/jmxfetch#95
[#96]: DataDog/jmxfetch#96
[#97]: DataDog/jmxfetch#97
[@alz]: https://github.com/alz
[@Bluestix]: https://github.com/bluestix
[@nwillems]: https://github.com/nwillems
yannmh added a commit to DataDog/dd-agent that referenced this pull request May 13, 2016
**Changes**
* [BUGFIX] Report properly beans with ':' in the name. See [#90][],
* [#91][], [#95][] (Thanks [@Bluestix][])
* [BUGFIX] Sanitize metric names and tags, i.e. remove illegal
* characters. See [#89][]
* [BUGFIX] Support `javax.management.Attribute` attribute types. See
* [#92][] (Thanks [@nwillems][])
* [FEATURE] Add user tags to service checks. See [#96][]
* [FEATURE] Allow group name substitutions in attribute/alias
* parameters. See [#94][], [#97][] (Thanks [@alz][])

<!--- The following link definition list is generated by PimpMyChangelog
--->
[#89]: DataDog/jmxfetch#89
[#90]: DataDog/jmxfetch#90
[#91]: DataDog/jmxfetch#91
[#92]: DataDog/jmxfetch#92
[#94]: DataDog/jmxfetch#94
[#95]: DataDog/jmxfetch#95
[#96]: DataDog/jmxfetch#96
[#97]: DataDog/jmxfetch#97
[@alz]: https://github.com/alz
[@Bluestix]: https://github.com/bluestix
[@nwillems]: https://github.com/nwillems
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.

None yet

2 participants