Support the empty_default_hostname instance field #184
Conversation
| @@ -51,7 +51,8 @@ | |||
| private Boolean cassandraAliasing; | |||
|
|
|||
| JMXAttribute(MBeanAttributeInfo attribute, ObjectName beanName, String instanceName, | |||
| Connection connection, HashMap<String, String> instanceTags, Boolean cassandraAliasing) { | |||
| Connection connection, HashMap<String, String> instanceTags, Boolean cassandraAliasing, | |||
There was a problem hiding this comment.
How do you feel we should handle the case where the instanceTags host tag is set (for whatever reason), but emptyDefaultHostname is true. IMHO it might make sense to just evict it from the HashMap and make the boolean option take precedence.
There was a problem hiding this comment.
For python/golang checks, we want to support an empty default hostname + check-provided hostname override (for kubernetes events node events / kube_state node metrics), so it's a legit use case.
It's currently not possible as an instance tag as it'll be sanitized to bean_host, but could it be added as a bean tag?
If so, do you think there's a better method to plug this logic into?
There was a problem hiding this comment.
Let's leave as-is, I think its probably fine.
| for (HashMap<String, Object> metric : metrics) { | ||
| String[] tags = (String[]) metric.get("tags"); | ||
| this.assertHostnameTags(Arrays.asList(tags)); | ||
| } |
There was a problem hiding this comment.
nit: this doesn't look like it's properly aligned 📏
| @@ -83,10 +84,16 @@ public Instance(LinkedHashMap<String, Object> instanceMap, LinkedHashMap<String, | |||
| // a jmxtree that is not completely initialized and would be missing some attributes | |||
| } | |||
|
|
|||
| this.minCollectionPeriod = (Integer) instanceMap.get("min_collection_interval"); | |||
| this.minCollectionPeriod = (Integer) this.instanceMap.get("min_collection_interval"); | |||
There was a problem hiding this comment.
Not really your fault, but we have so much room for NullPointerException in this constructor.....
There was a problem hiding this comment.
Indeed, though the same thing while working on it. Do you think we should wrap these in try/catch blocks?
There was a problem hiding this comment.
What about:
private boolean emptyDefaultHostname;
...
try {
this.emptyDefaultHostname = (Boolean) this.instanceMap.get("empty_default_hostname");
} catch (NullPointerException e) {
this.emptyDefaultHostname = false;
}
| if (this.minCollectionPeriod == null && initConfig != null) { | ||
| this.minCollectionPeriod = (Integer) initConfig.get("min_collection_interval"); | ||
| } | ||
|
|
||
| this.emptyDefaultHostname = (Boolean) this.instanceMap.get("empty_default_hostname"); |
There was a problem hiding this comment.
Again, not your fault as you're following a pattern used in the class, but I'd say it's safer to use boolean as the type for this.emptyDefaultHostname.
This makes jmxfetch submit metrics and service-checks with the dummy `host:` tag. It is supported for metrics since agent5, and for service-checks in 6.6
|
@truthbk rebased and addressed your feedback |
| @@ -51,7 +51,8 @@ | |||
| private Boolean cassandraAliasing; | |||
|
|
|||
| JMXAttribute(MBeanAttributeInfo attribute, ObjectName beanName, String instanceName, | |||
| Connection connection, HashMap<String, String> instanceTags, Boolean cassandraAliasing) { | |||
| Connection connection, HashMap<String, String> instanceTags, Boolean cassandraAliasing, | |||
There was a problem hiding this comment.
Let's leave as-is, I think its probably fine.
| } | ||
|
|
||
| @Test | ||
| public void testEmptyDefaultHostname() throws Exception { |
This makes jmxfetch submit metrics and service-checks with the dummy
host:tag for cluster checks. It is supported for metrics since agent5, and for service-checks in 6.6.See DataDog/datadog-agent#2334 and DataDog/datadog-agent#2351