Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HPCC-30446 esp components fail to start in cloud due to invalid metrics #17880

Merged
merged 1 commit into from
Oct 24, 2023
Merged

HPCC-30446 esp components fail to start in cloud due to invalid metrics #17880

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions esp/espcommon/espcommon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,13 @@ ESPCOMMON_API std::shared_ptr<hpccMetrics::ScaledHistogramMetric> registerServic
{
std::string metricName(processName);
metricName.append(".").append(serviceName).append(".").append(methodName);
auto no_ = std::remove(metricName.begin(), metricName.end(), '_');
metricName.erase(no_, metricName.end());

// Remove unwanted characters from new metric name
constexpr char removeChars[] = "_-* ";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: Where was this list derived from how about ! : or other characters which will be rejected later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

squash the commits and open a new issue if we see any real life problems.

for (unsigned i=0; i<strlen(removeChars); ++i)
{
metricName.erase(std::remove(metricName.begin(), metricName.end(), removeChars[i]), metricName.end());
}
return registerProfilingMetric(metricName.c_str(), desc, profilingOptions);
}

Expand Down
Loading