Implement OpenMetrics specification #9
Conversation
Pull Request Test Coverage Report for Build 2067817853
💛 - Coveralls |
ljbade
force-pushed
the
ljbade/om
branch
3 times, most recently
from
a50a1a0 to
523e1f8
Compare
ljbade
force-pushed
the
ljbade/rebase-upstream
branch
from
6837162 to
dd539fd
Compare
ljbade
force-pushed
the
ljbade/om
branch
3 times, most recently
from
6236e1f to
cca5b7d
Compare
silverjam
left a comment
silverjam
left a comment
There was a problem hiding this comment.
I think we should consider proposing this PR to the maintainers to gauge their interest pushing it upstream and see if they have any suggestions how to best structure the changes. Since OpenMetrics is supported by other tools (like ClickHouse) this seems like a feature that would be useful to other users.
If the maintainers are not interested in pulling in the changes we should consider how we are going to maintain this long term. If we structure these changes so that they lives alongside the existing code it might be easier to bring in future changes or bug fixes without a lot manual code wrangling.
| public: | ||
| using Serializer::Serialize; | ||
| std::string Serialize(const std::vector<MetricFamily>& metrics, | ||
| bool open_metrics) const; |
There was a problem hiding this comment.
Would it make sense to create an OpenMetrics specific serializer?
core/src/text_serializer.cc
Outdated
| for (auto& metric : family.metric) { | ||
| SerializeGauge(out, family, metric); | ||
| out << "# TYPE " << sorted_family.name << " gauge\n"; | ||
| if (!sorted_family.help.empty()) { |
There was a problem hiding this comment.
Is this something we always want to do regardless of the value of open_metrics?
There was a problem hiding this comment.
the order of type and help doesn't really matter, but in OpenMetrics I needed to ensure _total didn't end up in the help name for counters
core/src/text_serializer.cc
Outdated
| SerializeCounter(out, family, metric); | ||
| void SerializeFamily(std::ostream& out, const MetricFamily& family, | ||
| const bool open_metrics) { | ||
| auto ends_with = [](const std::string& value, |
There was a problem hiding this comment.
This feels different enough that it could be a new function instead of mutating the existing function. Do we still need to be able to write the old format?
There was a problem hiding this comment.
I don't think so as recent Prometheus versions handle OpenMetrics. Maybe you are right it might be easier to just upgrade to the new format.
ljbade
force-pushed
the
ljbade/rebase-upstream
branch
2 times, most recently
from
b389800 to
3fc5e1a
Compare
|
I wonder if its just easier to adopt the OpenMetrics format and remove the various conditions. Recent Prometheus versions understand the new format, so upstream concern might be breaking things for users of older Prometheus version but then it might be easier to stick to an old prometheus-cpp release. |
ljbade
force-pushed
the
ljbade/om
branch
2 times, most recently
from
bd99152 to
57d7d6b
Compare
This PR is set to merge on the rebased master, but I will update that once #7 is merged.
The OpenMetrics specification has a few changes from the previous Prometheus exposition format.
This PR adds a flag to allow enabling the OpenMetrics format. Which changes the following:
# EOFis added to the enduntypedmetric type is now calledunknowncountermetrics the value name must now end in_total(which was the recommended practice with Prometheus prior).countermetrics the name in the# TYPEheader must not contain_totalI have added a bunch of tests to check the changes.