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-31227 Address Prometheous histogram issues #18293

Merged
merged 1 commit into from
Feb 15, 2024
Merged

HPCC-31227 Address Prometheous histogram issues #18293

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
13 changes: 8 additions & 5 deletions system/metrics/sinks/prometheus/prometheusSink.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,9 @@ void PrometheusMetricSink::toPrometheusHistogram(const std::string &name, const
std::string PrometheusMetricSink::getPrometheusMetricName(const std::shared_ptr<IMetric> &pMetric)
{
std::string name = pMetric->queryName();
// Convert to lower case - per promtool output: "metric names should be written in 'snake_case' not 'camelCase'"
Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps out of scope of this issue, but lower casing all metric names per Prometheus promtool output:
dfs_8880_WsDfs_KeepAlive_seconds metric names should be written in 'snake_case' not 'camelCase'
dfs_8880_WsDfs_KeepAlive_seconds no help text

std::transform(name.begin(), name.end(), name.begin(),
[](unsigned char c){ return std::tolower(c); });

//'.' is a known char used in HPCC metric names but invalid in Prometheus
std::replace(name.begin(), name.end(), '.', '_');
Expand Down Expand Up @@ -240,16 +243,16 @@ const std::vector<std::string> &PrometheusMetricSink::getHistogramLabels(const s

// Add the inf label
std::string infLabel(name);
infLabel.append("_bucket{\"le=+Inf\"}");
infLabel.append("_bucket{le=\"+Inf\"}");
Copy link
Member Author

Choose a reason for hiding this comment

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

Incorrect double quote location

labels.emplace_back(infLabel);

// Sum and count
std::string sumLabel;
sumLabel.append("_sum");
std::string sumLabel(name);
Copy link
Member Author

Choose a reason for hiding this comment

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

_sum and _count missing metric name prefix

sumLabel.append("_sum{}");
labels.emplace_back(sumLabel);

std::string countLabel;
countLabel.append("_count");
std::string countLabel(name);
countLabel.append("_count{}");
Copy link
Contributor

@kenrowland kenrowland Feb 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Curious why the '{}" is present. Braces are not part of what is shown on the Prometheus page documenting their exposition formats. See https://prometheus.io/docs/instrumenting/exposition_formats/

Please double check the reason for "{}" in both places where added

Copy link
Member Author

Choose a reason for hiding this comment

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

Great observation, it's legal syntax as confirmed by promtool and prometheous graph UI used as a label container. It's optional, and honestly it was added as I tested why your histograms were deemed invalid.

Copy link
Member Author

Choose a reason for hiding this comment

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

What stands out to me is the fact there's currently no mechanism (at least not one I could see) to populate the histogram buckets w/ any available metadata as labels. That will likely need to be fixed.

labels.emplace_back(countLabel);

// Insert the labels for reuse
Expand Down
Loading