jmxtrans / jmxtrans-agent

Java Agent based JMX metrics exporter.
MIT License
178 stars 110 forks source link

Document/link to expression language in the README #73

Closed simmel closed 8 years ago

simmel commented 8 years ago

On some OSes, e.g. Ubuntu, getHostName() returns a shortname. Use getCanonicalHostName to always get an FQDN.

I thought I'd to the legwork and then have the discussion. What do you think about this change? For us it's very annoying to not be able to use #hostname# when submitting to Graphite, rather we have to create an ENV variable in the init-script and use that instead.

cyrille-leclerc commented 8 years ago

@simmel thanks for your proposal.

Could you please explain us why you did-not/could-not use #canonical_hostname# instead of #hostname#?

See https://github.com/jmxtrans/jmxtrans-agent/blob/efa75a3bee4c285e8d4f44f974131d653bfeb8b8/src/main/java/org/jmxtrans/agent/ExpressionLanguageEngineImpl.java#L52-L59

For backward portability reason, I have in mind to:

Would this make sense?

Side note, the compilation fails but I guess you have seen it:

https://travis-ci.org/jmxtrans/jmxtrans-agent/jobs/115810211

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.5:compile (default-compile) on project jmxtrans-agent: Compilation failure: Compilation failure:
[ERROR] /home/travis/build/jmxtrans/jmxtrans-agent/src/main/java/org/jmxtrans/agent/StatsDOutputWriter.java:[139,106] cannot find symbol
[ERROR] symbol:   method getCanonicalHostName()
[ERROR] location: class java.net.InetSocketAddress
[ERROR] /home/travis/build/jmxtrans/jmxtrans-agent/src/main/java/org/jmxtrans/agent/StatsDOutputWriter.java:[166,32] cannot find symbol
[ERROR] symbol:   method getCanonicalHostName()
[ERROR] location: variable address of type java.net.InetSocketAddress
[ERROR] /home/travis/build/jmxtrans/jmxtrans-agent/src/main/java/org/jmxtrans/agent/StatsDOutputWriter.java:[172,105] cannot find symbol
[ERROR] symbol:   method getCanonicalHostName()
[ERROR] location: variable address of type java.net.InetSocketAddress
[ERROR] /home/travis/build/jmxtrans/jmxtrans-agent/src/main/java/org/jmxtrans/agent/ExpressionLanguageEngineImpl.java:[45,48] non-static method getCanonicalHostName() cannot be referenced from a static context
[ERROR] /home/travis/build/jmxtrans/jmxtrans-agent/src/main/java/org/jmxtrans/agent/ExpressionLanguageEngineImpl.java:[45,69] incompatible types: java.lang.String cannot be converted to java.net.InetAddress
simmel commented 8 years ago

On Mon, 2016-03-14 at 02:54:35 -0700, Cyrille Le Clerc wrote:

Could you please explain us why you did-not/could-not use #canonical_hostname# instead of #hostname#? See https://github.com/jmxtrans/jmxtrans-agent/blob/efa75a3bee4c285e8d4f44f974131d653bfeb8b8/src/main/java/org/jmxtrans/agent/ExpressionLanguageEngineImpl.java#L52-L59

D'oh! I have never that or any of the other ones since they aren't mentioned in the README. What would be the best and non-must-keep-README-and-code-in-sync way to do that?

For backward portability reason, I have in mind to:

  • add a switch based on a system property to continue with the old mode
  • introduce a #short_hostname# function

Would this make sense?

Or just bump a major, if you use semver, and add a note to the README/release notes?

Really, I'm fine with you doing nothing about it and we using #canonical_hostname#.

Still committed to doing it? I can fix up my code and do the change if you mentor me a bit.

Side note, the compilation fails but I guess you have seen it:

I never tried the code tbh, I created a quick and dirty PR to start a discussion. = ) Didn't expect an answer this fast = )

cyrille-leclerc commented 8 years ago

Hello @simmel, the expression language is documented at https://github.com/jmxtrans/jmxtrans-agent/wiki/Expression-Language

We probably should make it more visible from the README.md page.

Really, I'm fine with you doing nothing about it and we using #canonical_hostname#.

As you can see in Expression Language, there is a logic between the expression language function and the java.net method. I would like to maintain this logic as much as possible.

Still committed to doing it? I can fix up my code and do the change if you mentor me a bit.

I suspect that the solution is just to improve the docs as the feature you ask for is here but was not enough documented.

I never tried the code tbh

Understood.

simmel commented 8 years ago

Something like that then?

btw, I noted that both the README and the wiki talks about ${graphite.host:2003} re: properties with default values. But shouldn't that be ${graphite.port:2003}?

cyrille-leclerc commented 8 years ago

Thanks @simmel . You are right for the default value, I'm fixing