Hi Frank,
On Tue, 2020-10-20 at 20:44 -0400, Frank Ch. Eigler via Elfutils-devel wrote:
> Subject: [PATCH 1/2] PR26756: add more prometheus metrics to debuginfod
>
> From: Frank Ch. Eigler <f...@redhat.com>
>
> Add an error_count{} family of metrics for each libc/libarchive/http
> exception instance created during operation. Add a family of fdcache*
> metrics for tracking fdcache operations and status. Test via a
> injecting a permission-000 empty nothing.rpm in the testsuite.
I found this somewhat hard to review since the indenting isn't
following the (GNU) style used in the rest of the file. Please place
curly brackets on their own line when possible.
> Signed-off-by: Frank Ch. Eigler <f...@redhat.com>
> ---
> debuginfod/ChangeLog | 6 ++++
> debuginfod/debuginfod.cxx | 56 +++++++++++++++++++++++++++-------
> --
> tests/ChangeLog | 6 ++++
> tests/run-debuginfod-find.sh | 12 ++++++--
> 4 files changed, 63 insertions(+), 17 deletions(-)
>
> diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
> index 8cb89967e9d1..236e2ca3270f 100644
> --- a/debuginfod/ChangeLog
> +++ b/debuginfod/ChangeLog
> @@ -1,3 +1,9 @@
> +2020-10-20 Frank Ch. Eigler <f...@redhat.com>
> +
> + PR26756: more prometheus metrics
> + * debuginfod.cxx (*_exception): Add counters for error occurrences.
> + (fdcache::*): Add counters for fdcache operations and status.
Also a new public set_metrics() function.
In general these changes look correct.
Unfortunately the test crashes on my RHEL7 with DTS9 system:
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x000000000040dd58 in std::string::size (this=0xe63910)
at /opt/rh/devtoolset-9/root/usr/include/c++/9/bits/basic_string.h:6226
6226 operator<(const basic_string<_CharT, _Traits, _Alloc>& __lhs,
(gdb) where
#0 0x000000000040dd58 in std::string::size (this=0xe63910)
at /opt/rh/devtoolset-9/root/usr/include/c++/9/bits/basic_string.h:6226
#1 std::string::compare (__str="fdcache_op_count{op=\"evict\"}",
this=0xe63910)
at /opt/rh/devtoolset-9/root/usr/include/c++/9/bits/basic_string.h:5753
#2 std::operator< <char, std::char_traits<char>, std::allocator<char> > (
__rhs="fdcache_op_count{op=\"evict\"}",
__lhs=<error reading variable: Cannot access memory at address 0x225c>)
at /opt/rh/devtoolset-9/root/usr/include/c++/9/bits/basic_string.h:6229
#3 std::less<std::string>::operator() (this=<optimized out>,
__y="fdcache_op_count{op=\"evict\"}",
__x=<error reading variable: Cannot access memory at address 0x225c>)
at /opt/rh/devtoolset-9/root/usr/include/c++/9/bits/stl_function.h:386
#4 std::_Rb_tree<std::string, std::pair<std::string const, long>,
std::_Select1st<std::pair<std::string const, long> >, std::less<std::string>,
std::allocator<std::pair<std::string const, long> > >::_M_lower_bound
(this=<optimized out>,
__k="fdcache_op_count{op=\"evict\"}", __y=0x7f922000ff00, __x=0xe638f0)
at /opt/rh/devtoolset-9/root/usr/include/c++/9/bits/stl_tree.h:1929
#5 std::_Rb_tree<std::string, std::pair<std::string const, long>,
std::_Select1st<std::pair<std::string const, long> >, std::less<std::string>,
std::allocator<std::pair<std::string const, long> > >::lower_bound (
__k="fdcache_op_count{op=\"evict\"}", this=0x435aa0 <metrics>)
at /opt/rh/devtoolset-9/root/usr/include/c++/9/bits/stl_tree.h:1282
#6 std::map<std::string, long, std::less<std::string>,
std::allocator<std::pair<std::string const, long> > >::lower_bound (
__x="fdcache_op_count{op=\"evict\"}", this=0x435aa0 <metrics>)
at /opt/rh/devtoolset-9/root/usr/include/c++/9/bits/stl_map.h:1258
#7 std::map<std::string, long, std::less<std::string>,
std::allocator<std::pair<std::string const, long> > >::operator[] (
__k="fdcache_op_count{op=\"evict\"}", this=0x435aa0 <metrics>)
at /opt/rh/devtoolset-9/root/usr/include/c++/9/bits/stl_map.h:495
#8 inc_metric (metric=..., lname=..., lvalue=...)
at /home/mark/src/elfutils/debuginfod/debuginfod.cxx:1716
#9 0x0000000000421db5 in libarchive_fdcache::limit (
this=this@entry=0x435ae0 <fdcache>, maxfds=maxfds@entry=0,
maxmbs=maxmbs@entry=0)
at /opt/rh/devtoolset-9/root/usr/include/c++/9/ext/new_allocator.h:80
#10 0x0000000000422760 in libarchive_fdcache::~libarchive_fdcache (
this=0x435ae0 <fdcache>, __in_chrg=<optimized out>)
at /home/mark/src/elfutils/debuginfod/debuginfod.cxx:1231
#11 0x00007f924ecb7ce9 in __run_exit_handlers (status=0,
listp=0x7f924f0456c8 <__exit_funcs>,
run_list_atexit=run_list_atexit@entry=true) at exit.c:77
#12 0x00007f924ecb7d37 in __GI_exit (status=<optimized out>) at exit.c:99
#13 0x00007f924eca055c in __libc_start_main (
main=0x40ad40 <main(int, char**)>, argc=10, argv=0x7ffc8ebaf6a8,
init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>,
stack_end=0x7ffc8ebaf698) at ../csu/libc-start.c:300
#14 0x000000000040c3e5 in _start ()
at /opt/rh/devtoolset-9/root/usr/include/c++/9/bits/stl_tree.h:211
Which seems to be caused by the libarchive_fdcache destructor calling
limit (0,0) triggering the new inc_metric which somehow cannot use the
metrics map?
> diff --git a/tests/ChangeLog b/tests/ChangeLog
> index 5a8b5899b9c5..aa68ffd383e8 100644
> --- a/tests/ChangeLog
> +++ b/tests/ChangeLog
> @@ -1,3 +1,9 @@
> +2020-10-20 Frank Ch. Eigler <f...@redhat.com>
> +
> + PR26756: more prometheus metrics
> + * run-debuginfod-find.sh: Trigger some errors with dummy "nothing.rpm"
> + and check for new metrics.
> +
> 2020-09-18 Mark Wielaard <m...@klomp.org>
>
> * run-readelf-compressed-zstd.sh: New test.
> diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
> index 730bb0e10b47..79976f70dc92 100755
> --- a/tests/run-debuginfod-find.sh
> +++ b/tests/run-debuginfod-find.sh
> @@ -95,6 +95,10 @@ wait_ready()
> fi
> }
>
> +# create a 000 empty .rpm file to evoke a metric-visible error
> +touch R/nothing.rpm
> +chmod 000 R/nothing.rpm
> +
Does this cause any issues during cleanup?
I would have expected something like:
if [ -f R/nothing.rpm ]; then chmod 644 R/nothing.rpm; fi
in cleanup(). But maybe the rm -f takes care of that?
Cheers,
Mark
