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.

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

Use a gauge for the histogram sum #386

Closed
wants to merge 1 commit into from
Closed

Use a gauge for the histogram sum #386

wants to merge 1 commit into from

Conversation

Neverlord
Copy link
Contributor

The online docs for Prometheus state:

The sum of observations (...) behaves like a counter, too, as long as
there are no negative observations.

A counter can only ever go up and asserts that users pass in only positive values when incrementing. Hence, the sum must use a gauge in order to stay consistent with the Prometheus documentation.

Source for the quote: https://prometheus.io/docs/practices/histograms/#count-and-sum-of-observations

The online docs for Prometheus state:

> The sum of observations (...) behaves like a counter, too, as long as
> there are no negative observations.

A counter can only ever go up and asserts that users pass in only
positive values when incrementing. Hence, the `sum` must use a gauge in
order to stay consistent with the Prometheus documentation.
@Neverlord
Copy link
Contributor Author

Btw, I don't think I can do something about the failed coverage check:

2020-07-03T16:44:58.5410598Z Traceback (most recent call last):
2020-07-03T16:44:58.5411044Z   File "/home/runner/.local/bin/coveralls", line 11, in <module>
2020-07-03T16:44:58.5411274Z     sys.exit(run())
2020-07-03T16:44:58.5412163Z   File "/home/runner/.local/lib/python2.7/site-packages/cpp_coveralls/__init__.py", line 92, in run
2020-07-03T16:44:58.5412546Z     raise ValueError("\nno coveralls.io token specified and no travis job id found\n"
2020-07-03T16:44:58.5413184Z ValueError: 
2020-07-03T16:44:58.5413702Z no coveralls.io token specified and no travis job id found
2020-07-03T16:44:58.5414335Z see --help for examples on how to specify a token
2020-07-03T16:44:58.5414539Z 
2020-07-03T16:44:58.5568476Z ##[error]Process completed with exit code 1.

@jupp0r
Copy link
Owner

jupp0r commented Jul 5, 2020

Would be nice if you could add a unit test to verify this works as expected.

@dota17
Copy link

dota17 commented Jul 6, 2020

I think the failed on coverage check is strange, there is nothing about travis job id, but log said it failed to find this.

btw, I also think the file range selected by Coverage is wrong:

Total Test time (real) =  20.39 sec
File '/home/runner/work/prometheus-cpp/prometheus-cpp/core/tests/gauge_test.cc'
Lines executed:100.00% of 63
Creating '#home#runner#work#prometheus-cpp#prometheus-cpp#_build_coverage#core#tests#CMakeFiles#prometheus_core_test.dir#gauge_test.cc.o###home#runner#work#prometheus-cpp#prometheus-cpp#core#tests#gauge_test.cc.gcov'

File '/usr/local/share/vcpkg/installed/x64-linux/include/gtest/internal/gtest-internal.h'
Lines executed:8.70% of 23
Creating '#home#runner#work#prometheus-cpp#prometheus-cpp#_build_coverage#core#tests#CMakeFiles#prometheus_core_test.dir#gauge_test.cc.o###usr#local#share#vcpkg#installed#x64-linux#include#gtest#internal#gtest-internal.h.gcov'

File '/usr/include/c++/5/iostream'
Lines executed:100.00% of 1
Creating '#home#runner#work#prometheus-cpp#prometheus-cpp#_build_coverage#core#tests#CMakeFiles#prometheus_core_test.dir#gauge_test.cc.o###usr#include#c++#5#iostream.gcov'

If you like, I'd love to make a little patch to fix this.

@Neverlord
Copy link
Contributor Author

Would be nice if you could add a unit test to verify this works as expected.

Will do. 🙂

gjasny added a commit that referenced this pull request Nov 1, 2020
@gjasny
Copy link
Collaborator

gjasny commented Nov 1, 2020

Closing in favour of #406.

@gjasny gjasny closed this Nov 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants