Gitlab Collector: Index size collector and test #835
Conversation
Signed-off-by: Felix Yuan <[email protected]>
There was a problem hiding this comment.
Thank you for your work on this collector and the other PRs that you have submitted. I know that it took a lot of time to put these together and I appreciate it. I have a few comments on this PR. I will try to get to the other PRs soon, but I think much of the feedback will be similar. My priority in reviewing the code is to consider how we can maintain this project long term. I am thrilled with the forward progress on this exporter but I don't want to sacrifice quality for the sake along the way.
As for the SQL formatting, I am particular about this. Poorly formatted SQL is much harder to read and understand. We do not have a style guide and I haven't seen a good formatter for SQL inside of go yet so I don't have anything good to help you directly. This site looks like it is close to what I would like to see: https://www.sqlstyle.guide/ but I haven't read it in detail yet. I think as long as we are close with style, we can make it work.
| if err := rows.Scan(&schemaname, &relname, &indexrelname, &indexSize); err != nil { | ||
| return err | ||
| } | ||
| schemanameLabel := "unknown" |
There was a problem hiding this comment.
I'm really not sure that these metrics even make sense if all of these are NULL. I understand that we used NULL to fix all of the bugs in the 0.13.1 release, but these are net new collectors and I would like to be much more thoughtful about how we handle them in code. For example, if the indexSize metric is NULL, why would we even report the metric? How is the zero value useful.
There was a problem hiding this comment.
Agreed, if the labels are null, we should Debug log and continue.
There was a problem hiding this comment.
Makes sense to me
There was a problem hiding this comment.
I think this code is still emitting metrics when these values are NULL.
Co-authored-by: Joe Adams <[email protected]> Signed-off-by: Felix Yuan <[email protected]>
Co-authored-by: Joe Adams <[email protected]> Signed-off-by: Felix Yuan <[email protected]>
| if err := rows.Scan(&schemaname, &relname, &indexrelname, &indexSize); err != nil { | ||
| return err | ||
| } | ||
| schemanameLabel := "unknown" |
There was a problem hiding this comment.
I think this code is still emitting metrics when these values are NULL.
| schemaname, | ||
| tablename AS relname, | ||
| indexname AS indexrelname, | ||
| pg_class.relpages * 8192::bigint AS index_size |
There was a problem hiding this comment.
We can't rely on 8192 being the block size. It's default, but could be changed. See references:
- https://www.postgresql.org/docs/current/catalog-pg-class.html (see relpages)
- https://www.postgresql.org/docs/current/runtime-config-preset.html (see block_size)
There was a problem hiding this comment.
ok so we have to select block size and multiply by that
Split from #819