HPCC-30446 esp components fail to start in cloud due to invalid metrics #17880
Conversation
|
https://track.hpccsystems.com/browse/HPCC-30446 |
|
@ghalliday Please merge |
There was a problem hiding this comment.
@kenrowland
I think the net effect of this change is to prevent the metric being flagged as invalid, but it will not actually affect the name of the metric that is reported.
Are you sure this code should not be in the caller?
system/jlib/jmetrics.cpp
Outdated
| @@ -153,6 +153,13 @@ bool MetricsManager::addMetric(const std::shared_ptr<IMetric> &pMetric) | |||
| bool rc = false; | |||
| std::string name = pMetric->queryName(); | |||
|
|
|||
| // Remove unwanted characters from input name | |||
There was a problem hiding this comment.
A couple of comments on this code:
i) It would possibly be better to not perform if the string doesn't contain any of those characters
ii) I think you could remove all characters in one go and avoid multiple passes/copies of the name. Something like
name.erase(std::remove_if(name.begin(), name.end(), [](unsigned char x) { strchr(removeChars, x) != nullptr }));
iii) Shouldn't this be the responsibility of the caller to pass a clean name?
There was a problem hiding this comment.
The case that caused the bug to be written is most likely because the ESP name from a helm chart (values.yaml probably) has a dash in it. The generated ESP service code for execution profiling uses the ESP process name when constructing metric names.
I considered adding code to the generated code to clean up the name prior to registering the metric, however a comment by Tony in the Jira suggested cleaning it up in the metrics code. The advantage of cleaning in the metrics code is it relieves all components from worrying about using names sourced from a configuration file (where a dash or other character may be legal) from having to clean the name prior to registering a metric.
If you prefer the caller clean the name first, I can add a utility function to the metrics framework and call it in the generated code prior to registration.
Your thoughts on pushing the responsibility to the caller?
There was a problem hiding this comment.
I think @afishbeck concern was that the service name should not be constrained by the rules of the metrics.
However it would make sense for registerServiceMethodProfilingMetric() to ensure that it is generating a valid metric name before it tries to register it.
It would make sense for the metric frame work either require a metric name is valid, or to clean it, but with this change it is doing both.
There was a problem hiding this comment.
Moved the removal to code in ESPCommon that registers the method profiling metric.
system/jlib/jmetrics.cpp
Outdated
| @@ -153,6 +153,13 @@ bool MetricsManager::addMetric(const std::shared_ptr<IMetric> &pMetric) | |||
| bool rc = false; | |||
| std::string name = pMetric->queryName(); | |||
|
|
|||
| // Remove unwanted characters from input name | |||
| char removeChars[] = "_-* "; | |||
kenrowland
force-pushed
the
HPCC-30446
branch
from
783ec34 to
185eb1d
Compare
| metricName.erase(no_, metricName.end()); | ||
|
|
||
| // Remove unwanted characters from new metric name | ||
| constexpr char removeChars[] = "_-* "; |
There was a problem hiding this comment.
minor: Where was this list derived from how about ! : or other characters which will be rejected later.
There was a problem hiding this comment.
@ghalliday
We could add more characters here, however this method is specific to creation of a profiling metric for ESP service methods. The intent is to remove any characters the code cannot control from sources where some characters are allowed, but are forbidden by the metrics framework. The sources in this case are the SCM files and the config itself (name of the ESP process).
For other uses of the framework, the component should be setting the metric name and ensuring it does not have illegal characters. In these cases, it might be the right thing to reject the metric lest the component owner thinks a metric has a character when the final registered metric does not. I can see the need for a separate function for cleaning a string that came from the config that could be used by components when building metric names,
Certainly we can take the approach that all metric names get scrubbed of illegal characters and add more to this list, but it would grow quite long since the regex for valid name essentially only allows upper and lower case letters, numbers, and periods (for hierarchical naming)
Thoughts?
There was a problem hiding this comment.
squash the commits and open a new issue if we see any real life problems.
There was a problem hiding this comment.
@kenrowland please squash
I'll merge as since it solves the immediate problem, but it is not clear that the list is sufficient.
| metricName.erase(no_, metricName.end()); | ||
|
|
||
| // Remove unwanted characters from new metric name | ||
| constexpr char removeChars[] = "_-* "; |
There was a problem hiding this comment.
squash the commits and open a new issue if we see any real life problems.
…trics Added code to metrics manager to remove illegal characters from metric name Signed-Off-By: Kenneth Rowland [email protected]
kenrowland
force-pushed
the
HPCC-30446
branch
from
185eb1d to
7dda535
Compare
|
@ghalliday Please merge |
ghalliday
merged commit 0c0a576
into
hpcc-systems:candidate-9.4.x
Added code to metrics manager to remove illegal characters from metric name
Signed-Off-By: Kenneth Rowland [email protected]
Type of change:
Checklist:
Smoketest:
Testing: