[ 
https://issues.apache.org/jira/browse/FLINK-7692?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16235098#comment-16235098
 ] 

Wei-Che Wei edited comment on FLINK-7692 at 11/7/17 5:36 AM:
-------------------------------------------------------------

Hi [~Zentol]

I proposed a draft for this issue and want to get some feedback from you.

A new {{KeyedGenericMetricGroup}} to support a new method 
{{MetricGroup#addGroup(String name, String value)}}.
{code}
class KeyedGenericMetricGroup extends AbstractMetricGroup {
  /** append name, value after parent.getScopeComponents()*/
  public GenericMetricGroup(MetricRegistry registry, AbstractMetricGroup 
parent, String name, String value)

  /** append (name -> value) to variables map */
  override public getAllVariables()
}
{code}

The Comparison between {{group.addGroup(name, value)}} and 
{{group.addGroup(name).addGroup(value)}}
| |group.addGroup(name, value)|group.addGroup(name).addGroup(value)|
|depth from parent| 1 | 2 |
|getMetricIdentifier()|\{parentIdentifier\}.name.value|\{parentIdentifier\}.name.value|
|getLogicalScope()|\{parentScope\}.name|\{parentScope\}.name.value|
|getAllVariables()|\{parentVariables\} ++ (name -> value)|\{parentVariables\}|

This can benefit the reporter such as Prometheus to use logical scope to 
aggregate the same type of metrics and distinguish each by variables.

There are some problems I met during designing this draft.
- Since {{group.addGroup(name, value)}} and 
{{group.addGroup(name).addGroup(value)}} have the same id of the returning 
metric group, there is only one metric group that will be registered. However, 
because they are not in the same layer, it is not easy to check if the other is 
exist. You should check it from your parent's or child's metric group. Is it 
acceptable to reject to register metric group from {{group.addGroup(name)}} 
when {{group.addGroup(name, value)}} has been called, vice versa?
- If the above is acceptable, what is the return value when we meet the 
conflict, because we don't want to throw RuntimeException on {{Metrics}} API 
and make the user program fail, right? I preferred to return 
{{group.addGroup(name, null)}} when called {{group.addGroup(name)}} after 
{{group.addGroup(name, some_value)}}; on the other hand, return 
{{group.addGroup(name).addGroup(value)}} and log the warning message.

What do you think? Are there other better approaches we can compare their pros 
and cons? Thank you.


was (Author: tonywei):
Hi [~Zentol]

I proposed a draft for this issue and want to get some feedback from you.

A new {{KeyedGenericMetricGroup}} to support a new method 
{{MetricGroup#addGroup(String name, String value)}}.
{code}
class KeyedGenericMetricGroup extends AbstractMetricGroup {
  /** append name, value after parent.getScopeComponents()*/
  public GenericMetricGroup(MetricRegistry registry, AbstractMetricGroup 
parent, String name, String value)

  /** append (name -> value) to variables map */
  override public getAllVariables()
}
{code}

The Comparison between {{group.addGroup(name, value)}} and 
{{group.addGroup(name).addGroup(value)}}
| |group.addGroup(name, value)|group.addGroup(name).addGroup(value)|
|depth from parent| 1 | 2 |
|getMetricIdentifier()|\{parentIdentifier\}.name.value|\{parentIdentifier\}.name.value|
|getLogicalScope()|\{parentScope\}.name|\{parentScope\}.name.value|
|getAllVariables()|\{parentVariables\} ++ (name -> value)|\{parentVariables\}|

This can benefit the reporter such as Prometheus to use logical scope to 
aggregate the same type of metrics and distinguish each by variables.

There are some problems I met during designing this draft.
- Since {{group.addGroup(name, value)}} and 
{{group.addGroup(name).addGroup(value)}} have the same id of the returning 
metric group, there is only one metric group that will be registered. However, 
because they are not in the same layer, it is not easy to check if the other is 
exist. You should check it from your parent's or child's metric group. Is it 
acceptable to reject to register metric group from {{group.addGroup(name)}} 
when {{group.addGroup(name, value)}} has been called, vice versa?
- If the above is acceptable, what is the return value when we meet the 
conflict, because we don't want to throw RuntimeException on {{Metrics}} API 
and make the user program fail, right? I preferred to return 
{{group.addGroup(name, null)}} when called {{group.addGroup(name)}} after 
{{group.addGroup(name, some_value)}}; on the other hand, return 
{{group.addGroup(name).addGroup(value) and log the warning message.

What do you think? Are there other better approaches we can compare their pros 
and cons? Thank you.

> Support user-defined variables in Metrics
> -----------------------------------------
>
>                 Key: FLINK-7692
>                 URL: https://issues.apache.org/jira/browse/FLINK-7692
>             Project: Flink
>          Issue Type: Improvement
>          Components: Metrics
>    Affects Versions: 1.4.0
>            Reporter: Chesnay Schepler
>            Assignee: Wei-Che Wei
>            Priority: Minor
>             Fix For: 1.4.0
>
>
> Reporters that identify metrics with a set of key-value pairs are currently 
> limited to the variables defined by Flink, like the taskmanager ID, with 
> users not being able to supply their own.
> This is inconsistent with reporters that use metric identifiers that freely 
> include user-defined groups constructted via {{MetricGroup#addGroup(String 
> name)}}.
> I propose adding a new method {{MetricGroup#addGroup(String key, String 
> name)}} that adds a new key-value pair to the {{variables}} map in it's 
> constructor. When constructing the metric identifier the key should be 
> included as well, resulting in the same result as when constructing the 
> metric groups tree via {{group.addGroup(key).addGroup(value)}}.
> For this a new {{KeyedGenericMetricGroup}} should be created that resembles 
> the unkeyed version, with slight modifications to the constructor and 
> {{getScopeComponents}} method.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

Reply via email to