[GitHub] [flink] zentol commented on pull request #14510: [FLINK-19656] [metrics] filter delimiter in metrics components

[GitBox <gi...@apache.org>]

zentol commented on pull request #14510:
URL: https://github.com/apache/flink/pull/14510#issuecomment-751770942


   well...this isn't quite the direction I was going for.
   
   The problem I have with it is that we have to modify every single reporter.
   This has a few implications:
   - this behavior is now essentially opt-in, which ultimately means that user-defined reporters are likely to behave differently, still suffering from the problem at hand
   - duplicate logic in reporters
     - which you reduce via inheritance, but:
       - it is now hard-wired to go through the `open()` method, which is essentially legacy code (because new reporters should use factories and pass everything via the constructor, but you can't force that in `AbstractReporter` because legacy reporters may still be around)
       - it is super easy to miss a call to `super#filterCharacters`
       - forces reporters to now pass themselves as the `CharacterFilter`, which is a pattern that we should have never introduced
    - if the new filter actually modifies values then the caching mechanism in the metric groups is rendered pointless (this was admittedly already the case for _some_ reporters, but it is something we want to change in FLINK-14410)
   
   I can just echo my previous statement in the JIRA; we should modify the FrontMetricGroup instead to wrap the given filter (or introduce one where the filter is null) doing the replacement, and add a ( #getAllVariables()) variant that accepts a filter.
   Then this behavior will be consistent for every reporter, without having to touch any of them.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org