[GitHub] flink pull request #4110: [FLINK-6900] [metrics] Limit size of metric name c...
Github user NicoK commented on a diff in the pull request:
https://github.com/apache/flink/pull/4110#discussion_r126940120
--- Diff:
flink-metrics/flink-metrics-statsd/src/test/java/org/apache/flink/metrics/statsd/StatsDReporterTest.java
---
@@ -60,6 +60,20 @@
public class StatsDReporterTest extends TestLogger {
@Test
+ public void testNameTruncating() {
+ StatsDReporter reporter = new StatsDReporter();
+
+ MetricConfig config = new MetricConfig();
+ config.setProperty(StatsDReporter.ARG_HOST, "localhost");
+ config.setProperty(StatsDReporter.ARG_PORT, "12345");
+ config.setProperty(StatsDReporter.ARG_MAX_COMPONENT_LENGTH,
"10");
+
+ reporter.open(config);
+
+ assertEquals("0123456789",
reporter.filterCharacters("0123456789DEADBEEF"));
--- End diff --
hmm, seems something was lost during the transfer :(
iirc, I wanted to ask whether it makes sense to also test that things like
`a.b.0123456789DEADBEEF.c` are properly truncated to `a.b.0123456789.c` for the
whole metric name
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4110: [FLINK-6900] [metrics] Limit size of metric name c...
Github user greghogan commented on a diff in the pull request: https://github.com/apache/flink/pull/4110#discussion_r126930326 --- Diff: docs/monitoring/metrics.md --- @@ -376,6 +376,7 @@ Parameters: - `dmax` - hard limit for how long an old metric should be retained - `ttl` - time-to-live for transmitted UDP packets - `addressingMode` - UDP addressing mode to use (UNICAST/MULTICAST) +- `maxComponentLength` - limits the size of each scope component --- End diff -- Alright, works for me. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request #4110: [FLINK-6900] [metrics] Limit size of metric name c...
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4110#discussion_r126915402
--- Diff:
flink-metrics/flink-metrics-statsd/src/test/java/org/apache/flink/metrics/statsd/StatsDReporterTest.java
---
@@ -60,6 +60,20 @@
public class StatsDReporterTest extends TestLogger {
@Test
+ public void testNameTruncating() {
+ StatsDReporter reporter = new StatsDReporter();
+
+ MetricConfig config = new MetricConfig();
+ config.setProperty(StatsDReporter.ARG_HOST, "localhost");
+ config.setProperty(StatsDReporter.ARG_PORT, "12345");
+ config.setProperty(StatsDReporter.ARG_MAX_COMPONENT_LENGTH,
"10");
+
+ reporter.open(config);
+
+ assertEquals("0123456789",
reporter.filterCharacters("0123456789DEADBEEF"));
--- End diff --
What about the tests?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4110: [FLINK-6900] [metrics] Limit size of metric name c...
Github user zentol commented on a diff in the pull request: https://github.com/apache/flink/pull/4110#discussion_r126918307 --- Diff: docs/monitoring/metrics.md --- @@ -376,6 +376,7 @@ Parameters: - `dmax` - hard limit for how long an old metric should be retained - `ttl` - time-to-live for transmitted UDP packets - `addressingMode` - UDP addressing mode to use (UNICAST/MULTICAST) +- `maxComponentLength` - limits the size of each scope component --- End diff -- "length of each scope component" would be better, I don't think we us "name of scope component" anywhere in the docs. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request #4110: [FLINK-6900] [metrics] Limit size of metric name c...
Github user NicoK commented on a diff in the pull request:
https://github.com/apache/flink/pull/4110#discussion_r126443494
--- Diff:
flink-metrics/flink-metrics-statsd/src/main/java/org/apache/flink/metrics/statsd/StatsDReporter.java
---
@@ -193,7 +198,10 @@ private void send(final String name, final String
value) {
@Override
public String filterCharacters(String input) {
char[] chars = null;
- final int strLen = input.length();
+ if (input.length() > maxComponentLength) {
+ log.warn("The metric name component {} exceeded the {}
characters length limit and was truncated.", input, maxComponentLength);
+ }
+ final int strLen = Math.min(input.length(), maxComponentLength);
--- End diff --
same here about the `Math.min`
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4110: [FLINK-6900] [metrics] Limit size of metric name c...
Github user greghogan commented on a diff in the pull request: https://github.com/apache/flink/pull/4110#discussion_r126446702 --- Diff: docs/monitoring/metrics.md --- @@ -376,6 +376,7 @@ Parameters: - `dmax` - hard limit for how long an old metric should be retained - `ttl` - time-to-live for transmitted UDP packets - `addressingMode` - UDP addressing mode to use (UNICAST/MULTICAST) +- `maxComponentLength` - limits the size of each scope component --- End diff -- Could this be described as the "length of the name of each scope component" rather than simply "size"? I'm not sure that it's immediately obvious what this parameter is limiting. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request #4110: [FLINK-6900] [metrics] Limit size of metric name c...
Github user NicoK commented on a diff in the pull request:
https://github.com/apache/flink/pull/4110#discussion_r126442516
--- Diff:
flink-metrics/flink-metrics-dropwizard/src/main/java/org/apache/flink/dropwizard/ScheduledDropwizardReporter.java
---
@@ -184,7 +188,10 @@ public void notifyOfRemovedMetric(Metric metric,
String metricName, MetricGroup
@Override
public String filterCharacters(String metricName) {
char[] chars = null;
- final int strLen = metricName.length();
+ if (metricName.length() > maxComponentLength) {
+ log.warn("The metric name component {} exceeded the {}
characters length limit and was truncated.", metricName, maxComponentLength);
+ }
+ final int strLen = Math.min(metricName.length(),
maxComponentLength);
--- End diff --
You actually don't need to call `Math.min()` anymore after you already
checked the condition for the warning message. You could thus assign `strLen`
yourself.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4110: [FLINK-6900] [metrics] Limit size of metric name c...
Github user NicoK commented on a diff in the pull request:
https://github.com/apache/flink/pull/4110#discussion_r126443486
--- Diff:
flink-metrics/flink-metrics-statsd/src/test/java/org/apache/flink/metrics/statsd/StatsDReporterTest.java
---
@@ -60,6 +60,20 @@
public class StatsDReporterTest extends TestLogger {
@Test
+ public void testNameTruncating() {
+ StatsDReporter reporter = new StatsDReporter();
+
+ MetricConfig config = new MetricConfig();
+ config.setProperty(StatsDReporter.ARG_HOST, "localhost");
+ config.setProperty(StatsDReporter.ARG_PORT, "12345");
+ config.setProperty(StatsDReporter.ARG_MAX_COMPONENT_LENGTH,
"10");
+
+ reporter.open(config);
+
+ assertEquals("0123456789",
reporter.filterCharacters("0123456789DEADBEEF"));
--- End diff --
...and the additional tests
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4110: [FLINK-6900] [metrics] Limit size of metric name c...
GitHub user zentol opened a pull request: https://github.com/apache/flink/pull/4110 [FLINK-6900] [metrics] Limit size of metric name components This PR modifies the `ScheduledDropwizardReporter` to limit the size of every metric name component to 80 characters, with the same reasoning as #4109. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zentol/flink 6900 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/4110.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 #4110 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
