You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by zentol <gi...@git.apache.org> on 2016/08/25 13:28:02 UTC
[GitHub] flink pull request #2418: [FLINK-4245] JMXReporter exposes all defined varia...
GitHub user zentol opened a pull request:
https://github.com/apache/flink/pull/2418
[FLINK-4245] JMXReporter exposes all defined variables
This PR is a follow-up to cbbd80033204a922ccf148c5ac3f1be6de0d810d. It exposes the introduced map of variables via JMX by adding a new method `getVariables()` to the JMX metric interfaces.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zentol/flink metrics_tags_jmx
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/2418.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2418
----
commit be55aa1bdaf8a574ba2a34786507a691af748dde
Author: zentol <ch...@apache.org>
Date: 2016-07-27T10:29:10Z
[FLINK-4245] JMXReporter exposes all defined variables
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2418: [FLINK-4245] JMXReporter exposes all defined variables
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2418
We could lazily filter them in the `get(...)` methods.
But looking at how much work `ObjectName` does internally, it seems like making JMX a zero overhead thing is a bit of a lost cause...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2418: [FLINK-4245] JMXReporter exposes all defined variables
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2418
Yes, that makes sense and can be done. However, we still have the issue that the keys/values in the variables map may not be JMX compliant. We may have to recreate the Hashtable after all.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2418: [FLINK-4245] JMXReporter exposes all defined variables
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2418
I've updated the PR.
* reverted back to creating a new Hashtable and filtering all keys and values
* introduced the notion of a "logical scope"; in other words the group hierarchy without specific information like ID's and such. Example: "taskmanager.job.task.usergroup". The order is strictly the group hierarchy and unaffected by the scope formats.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2418: [FLINK-4245] JMXReporter exposes all defined variables
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2418
How about this: Let's create the JMX metrics with properly filtered names. Re-creating the Hashtable is what it is.
If we see this becoming an issue, we can actually have a background thread do the initialization of JMX metrics (poll from a registration queue and create the beans).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2418: [FLINK-4245] JMXReporter exposes all defined variables
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2418
oh no....the subgroups are missing at the moment; the only thing exposed are the system variables.
This is a problem :/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2418: [FLINK-4245] JMXReporter exposes all defined variables
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2418
Well this won't be pretty. The problem lies with the "name" check we have to do since we mustn't modify the underlying map, since it belongs to the metric groups.
As you showed in your example we need a check in `HashtableWrapper#get()`.
But we also have to make sure that this check is done for all possible ways the Hashtable values could get accessed.
This includes the `Sets` returned by `keySet()`, `values` and `entrySet` and the `Iterators` returned by these `Sets`.
I've pushed a version that does exactly this. One issue I've already found is that we can no longer filter out characters from the variables.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2418: [FLINK-4245] JMXReporter exposes all defined variables
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2418
The string would be constant though, for all types of the same metric.
Users would select the metric via "domain", let's say "flink.taskmanager.task.operator.numBytesOut". Then the user would use the tags to filter which operator and taskmanager they would be interested in, for example filer "hostname=worker7 & operatorname=GeoTagMapper".
Does that make sense?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2418: [FLINK-4245] JMXReporter exposes all defined variables
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2418
Looks good to me!
The only not-so-nice thing is that we have to convert the `Map` to a `HashTable` for every metric. Given how we tried to optimize the metric creation overhead, this it pretty tough.
Its seems very hard to "disguise" a map as a hashtable, though...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2418: [FLINK-4245] JMXReporter exposes all defined variables
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2418
Can the concatenation of the groups not be the "domain"? Currently, the domain is static, but it does not have to be static, I think.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2418: [FLINK-4245] JMXReporter exposes all defined varia...
Posted by mushketyk <gi...@git.apache.org>.
Github user mushketyk commented on a diff in the pull request:
https://github.com/apache/flink/pull/2418#discussion_r76802688
--- Diff: flink-metrics/flink-metrics-jmx/src/main/java/org/apache/flink/metrics/jmx/JMXReporter.java ---
@@ -143,23 +144,27 @@ public int getPort() {
@Override
public void notifyOfAddedMetric(Metric metric, String metricName, MetricGroup group) {
- final String name = generateJmxName(metricName, group.getScopeComponents());
+ final Hashtable<String, String> table = generateJmxTable(metricName, group.getAllVariables());
--- End diff --
Oh, I see ObjectName requires it. My bad.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2418: [FLINK-4245] JMXReporter exposes all defined variables
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2418
Actually, it may be possible, and not too bad if we assume the Hashtable is immutable. Something like this:
```java
public class HashtableWrapper extends Hashtable<String, String> {
private final Map<String, String> backingMap;
private final String name;
public HashtableWrapper(Map<String, String> backingMap, String name) {
super(1);
this.backingMap = backingMap;
this.name = name;
}
@Override
public synchronized V get(Object key) {
if ("name".equals(key)) {
return name;
} else {
return backingMap.get(key);
}
}
@Override
public synchronized String put(String key, String value) {
throw new UnsupportedOperationException("immutable hashtable");
}
// wrappers for Iterator to Enumeration
@Override
public synchronized Enumeration<String> keys() {
return new IteratorToEnumeration(backingMap.keySet());
}
@Override
public synchronized Enumeration<String> elements() {
return new IteratorToEnumeration(backingMap.valueSet());
}
// and so on ...
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2418: [FLINK-4245] JMXReporter exposes all defined varia...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/2418
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2418: [FLINK-4245] JMXReporter exposes all defined variables
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2418
Looks good, +1 to add this with minor comments.
Two comments inline, and I would suggest to call `getLogicalName()` simply `getName()` or `getGroupName()`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2418: [FLINK-4245] JMXReporter exposes all defined varia...
Posted by mushketyk <gi...@git.apache.org>.
Github user mushketyk commented on a diff in the pull request:
https://github.com/apache/flink/pull/2418#discussion_r76802305
--- Diff: flink-metrics/flink-metrics-jmx/src/main/java/org/apache/flink/metrics/jmx/JMXReporter.java ---
@@ -143,23 +144,27 @@ public int getPort() {
@Override
public void notifyOfAddedMetric(Metric metric, String metricName, MetricGroup group) {
- final String name = generateJmxName(metricName, group.getScopeComponents());
+ final Hashtable<String, String> table = generateJmxTable(metricName, group.getAllVariables());
--- End diff --
Out of curiosity. Why do we use Hashtable instead of HashMap?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2418: [FLINK-4245] JMXReporter exposes all defined variables
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2418
I think the code looks pretty good.
Is the name of the group metric group somehow reflected in the reporter? Meaning that if users have a cascade of subgroups, this is somehow shown in the domain or in one of the tags?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2418: [FLINK-4245] JMXReporter exposes all defined variables
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2418
it doesn't _have_ to be static., but then we would be back to square one with users having to parse values from a string (the domain).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2418: [FLINK-4245] JMXReporter exposes all defined variables
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2418
ok.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2418: [FLINK-4245] JMXReporter exposes all defined variables
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2418
Looks like we could just pop the variables map into constructor and get the key names for free, that's neat.
I wouldn't worry too much about disregarding the naming scheme; in contrast to other reporters additional keys (as i understand it) don't make it more difficult to work with a given metric.
What we could do is add a configuration option `exposeAllKeys` or something that if set pops the map into the constructor, otherwise it uses the configured naming scheme.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2418: [FLINK-4245] JMXReporter exposes all defined variables
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2418
I think that works to expose the names. I was wondering, though, if we can have a more JMX-natural integration.
The JMX `ObjectName` has the constructor `ObjectName(String domain, Hashtable<String,String> table)`. Rather than having `key1=taskmanager,key2=<hostname>,key3=<jobname>`, we could go for `domain=taskmanager.task.operator.io.numRecords` (no variable parsing) and `table={taskmanager=<tid>, hostname=<hostname>, jobid=<jid>, ...`. If I got the thoughts of some of the users right, that would be the preferred way for them.
That probably contradicts the "configurable name" design we have for the other reporters, because the table and the formatted scope/name override each other (I think, not sure, not a JMX expert here).
In the end, both approaches are probably valid and which one is more usable depends on the metric infrastructure of the users. So, if we cannot get both into the same reporter, should we simply have two JMX reporters? A "tag-based" (like discussed above) and a "name/scope-based" (like we have so far)?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2418: [FLINK-4245] JMXReporter exposes all defined variables
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2418
The funky thing is that internally the ObjectName reconverts the Hashtable to a Map...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2418: [FLINK-4245] JMXReporter exposes all defined varia...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/2418#discussion_r85146695
--- Diff: flink-metrics/flink-metrics-jmx/src/main/java/org/apache/flink/metrics/jmx/JMXReporter.java ---
@@ -206,27 +211,24 @@ public void notifyOfRemovedMetric(Metric metric, String metricName, MetricGroup
// Utilities
// ------------------------------------------------------------------------
- static String generateJmxName(String metricName, String[] scopeComponents) {
- final StringBuilder nameBuilder = new StringBuilder(128);
- nameBuilder.append(PREFIX);
-
- for (int x = 0; x < scopeComponents.length; x++) {
- // write keyX=
- nameBuilder.append(KEY_PREFIX);
- nameBuilder.append(x);
- nameBuilder.append("=");
-
- // write scope component
- nameBuilder.append(replaceInvalidChars(scopeComponents[x]));
- nameBuilder.append(",");
+ static Hashtable<String, String> generateJmxTable(Map<String, String> variables) {
+ Hashtable<String, String> ht = new Hashtable<>(variables.size());
+ for (Map.Entry<String, String> variable : variables.entrySet()) {
+ ht.put(replaceInvalidChars(variable.getKey()), replaceInvalidChars(variable.getValue()));
}
+ return ht;
+ }
- // write the name
- nameBuilder.append("name=").append(replaceInvalidChars(metricName));
-
- return nameBuilder.toString();
+ static String generateJmxDomain(String metricName, MetricGroup group) {
+ CharacterFilter filter = new CharacterFilter() {
--- End diff --
I think this can be a single instance reused across all calls to "generateJmxDomain" in the JMX reporter.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2418: [FLINK-4245] JMXReporter exposes all defined varia...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/2418#discussion_r85145059
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/AbstractMetricGroup.java ---
@@ -112,6 +116,43 @@ public AbstractMetricGroup(MetricRegistry registry, String[] scope, A parent) {
}
/**
+ * Returns the logical scope of this group, for example
+ * {@code "taskmanager.job.task"}
+ *
+ * @param filter character filter which is applied to the scope components
+ * @return logical scope
+ */
+ public String getLogicalScope(CharacterFilter filter) {
+ return getLogicalScope(filter, registry.getDelimiter());
+ }
+
+ /**
+ * Returns the logical scope of this group, for example
+ * {@code "taskmanager.job.task"}
+ *
+ * @param filter character filter which is applied to the scope components
+ * @return logical scope
+ */
+ public String getLogicalScope(CharacterFilter filter, Character delimiter) {
--- End diff --
Can this be a primitive `char`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2418: [FLINK-4245] JMXReporter exposes all defined variables
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2418
The wrapper is a nice idea, will look into it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---