[FLINK-6900] [metrics] Limit size of metric name components #4110
Conversation
|
yes it is still required, but this PR does address a similar issue. #4109 limits the size of the operator name in the metric identifier. This was a problem for all reporters, because a 200+ character name just isn't manageable. This PR limits the size of all components of the metric identifier for DropwizardReporters, as the backends of several subclasses store metrics in files, with each component being one directory., like "taskmanager/abcde/job/myjob/task/mytask". Since they are used as names for directories they mustn't exceed a certain size (commonly 255). While technically a value close to 255 would suffice, i figure that anything above 80 characters isn't really well manageable as well. |
|
What can cause such a long metric identifier? It seems risky to truncate the full identifier which could even completely remove the base name. |
|
We don't truncate the full identifier, but each individual component before assembling the final identifier. I.e. The primary cause of this currently are the names of WindowOperators or long task chains; but given that the sections are partially controlled by the user there may be more cases. |
|
That sounds reasonable. Can we add a warning as in #4109 and replace |
|
sure, i can do that. |
|
just for clarification, |
|
@NicoK yes, that's how they would be stored in some backends. |
|
@zentol then +1 after addressing @greghogan's comments (adding a warning + using a constant for the |
|
I've update the PR.
|
NicoK
left a comment
NicoK
left a comment
There was a problem hiding this comment.
good changes + tests.
+1 after addressing some more comments
| 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); |
There was a problem hiding this comment.
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.
| config.setProperty(StatsDReporter.ARG_MAX_COMPONENT_LENGTH, "10"); | ||
|
|
||
| reporter.open(config); | ||
|
|
There was a problem hiding this comment.
What about the tests?
There was a problem hiding this comment.
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 (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); |
There was a problem hiding this comment.
same here about the Math.min
docs/monitoring/metrics.md
Outdated
| - `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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
"length of each scope component" would be better, I don't think we us "name of scope component" anywhere in the docs.
4 participants
This PR modifies the
ScheduledDropwizardReporterto limit the size of every metric name component to 80 characters, with the same reasoning as #4109.