Uploaded image for project: 'Flink'
  1. Flink
  2. FLINK-6898

Limit size of operator component in metric name

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 1.3.0, 1.4.0
    • Fix Version/s: 1.4.0, 1.3.2
    • Component/s: Metrics
    • Labels:
      None

      Description

      The operator name for some operators (specifically windows) can be very, very long (250+) characters.

      I propose to limit the total space that the operator component can take up in a metric name to 60 characters.

        Issue Links

          Activity

          Hide
          Zentol Chesnay Schepler added a comment -

          1.3: 1d2c615853ffa627a61de6c9880c53c3b00f1e31
          1.4: edb79b0a8bb2b33e1a0470b99b11274ac0e9c673

          Show
          Zentol Chesnay Schepler added a comment - 1.3: 1d2c615853ffa627a61de6c9880c53c3b00f1e31 1.4: edb79b0a8bb2b33e1a0470b99b11274ac0e9c673
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/flink/pull/4109

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/4109
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on the issue:

          https://github.com/apache/flink/pull/4109

          a constant is a good idea, will add it and start merging.

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on the issue: https://github.com/apache/flink/pull/4109 a constant is a good idea, will add it and start merging.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user greghogan commented on the issue:

          https://github.com/apache/flink/pull/4109

          Update looks good. Do we want to define `80` as a constant?

          Show
          githubbot ASF GitHub Bot added a comment - Github user greghogan commented on the issue: https://github.com/apache/flink/pull/4109 Update looks good. Do we want to define `80` as a constant?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on the issue:

          https://github.com/apache/flink/pull/4109

          no, it's not safe nor does it actually fulfill the purpose. I better add a test for this....

          We can't just fail, as the metric system shouldn't throw exceptions, since this isn't a problem that affects job execution.

          I've moved the truncation into `TaskMetricGroup#addOperator()` which is the only call to the constructor outside of tests (as intended).

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on the issue: https://github.com/apache/flink/pull/4109 no, it's not safe nor does it actually fulfill the purpose. I better add a test for this.... We can't just fail, as the metric system shouldn't throw exceptions, since this isn't a problem that affects job execution. I've moved the truncation into `TaskMetricGroup#addOperator()` which is the only call to the constructor outside of tests (as intended).
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user greghogan commented on the issue:

          https://github.com/apache/flink/pull/4109

          Is it safe to set the full `operatorName` in the parent constructor scope but truncate in the local info and variables map? Would it be better to explicitly fail rather than implicitly truncate?

          Show
          githubbot ASF GitHub Bot added a comment - Github user greghogan commented on the issue: https://github.com/apache/flink/pull/4109 Is it safe to set the full `operatorName` in the parent constructor scope but truncate in the local info and variables map? Would it be better to explicitly fail rather than implicitly truncate?
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user zentol opened a pull request:

          https://github.com/apache/flink/pull/4109

          FLINK-6898 [metrics] Limit size of operator component in metric name

          This PR prevents the operator name component of a metric name to exceed 80 characters. I picked 80 since it is below 255 (a common maximum for file lengths), but still large enough for that once-in-a-million special case. I guess.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/zentol/flink 6898

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/flink/pull/4109.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 #4109


          commit 71bf096d6562d45a3b235ad8c027cdd601f6d937
          Author: zentol <chesnay@apache.org>
          Date: 2017-06-12T13:36:25Z

          FLINK-6898 [metrics] Limit size of operator component in metric name


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user zentol opened a pull request: https://github.com/apache/flink/pull/4109 FLINK-6898 [metrics] Limit size of operator component in metric name This PR prevents the operator name component of a metric name to exceed 80 characters. I picked 80 since it is below 255 (a common maximum for file lengths), but still large enough for that once-in-a-million special case. I guess. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zentol/flink 6898 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/4109.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 #4109 commit 71bf096d6562d45a3b235ad8c027cdd601f6d937 Author: zentol <chesnay@apache.org> Date: 2017-06-12T13:36:25Z FLINK-6898 [metrics] Limit size of operator component in metric name

            People

            • Assignee:
              Zentol Chesnay Schepler
              Reporter:
              Zentol Chesnay Schepler
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development