[Asterisk-code-review] Add core Prometheus support to Asterisk (...asterisk[master])

[Matt Jordan]

Matt Jordan has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/11337 )

Change subject: Add core Prometheus support to Asterisk
......................................................................


Patch Set 8:

(10 comments)

Thanks for the review! Should have everything corrected.

https://gerrit.asterisk.org/#/c/11337/7/res/res_prometheus.c 
File res/res_prometheus.c:

https://gerrit.asterisk.org/#/c/11337/7/res/res_prometheus.c@283 
PS7, Line 283: 
> Should have a check for NULL metric.
Ack


https://gerrit.asterisk.org/#/c/11337/7/res/res_prometheus.c@295 
PS7, Line 295: 		AST_LIST_TRAVERSE_SAFE_BEGIN(&existing->children, child, entry) {
> Not modifying the linked list so _SAFE_ isn't needed.
Ack


https://gerrit.asterisk.org/#/c/11337/7/res/res_prometheus.c@314 
PS7, Line 314: 	AST_VECTOR_APPEND(&metrics, metric);
> Can fail if vector needs to grow.
Ack


https://gerrit.asterisk.org/#/c/11337/7/res/res_prometheus.c@333 
PS7, Line 333: 			if (!existing) {
> I don't think this can happen.
Ack


https://gerrit.asterisk.org/#/c/11337/7/res/res_prometheus.c@350 
PS7, Line 350: 						AST_LIST_INSERT_TAIL(&root->children, child, entry);
> So the first child of the removed metric gets nominated to be the new parent of the rest of the list […]
Correct. Metrics are all siblings of each other, where the 'parent' is really just the oldest sibling. If we happen to remove it, we need to promote a new one to the front of the line.


https://gerrit.asterisk.org/#/c/11337/7/res/res_prometheus.c@527 
PS7, Line 527: 
> Probably want to check that callback->name and callback->callback_fn aren't NULL;
Ack


https://gerrit.asterisk.org/#/c/11337/7/res/res_prometheus.c@541 
PS7, Line 541: 		if (!entry) {
> Don't think this can happen.
Ack


https://gerrit.asterisk.org/#/c/11337/7/res/res_prometheus.c@601 
PS7, Line 601: 		if (!callback) {
> Don't think this can happen.
Ack


https://gerrit.asterisk.org/#/c/11337/7/res/res_prometheus.c@611 
PS7, Line 611: 		if (!metric) {
> Don't think this can happen.
Ack


https://gerrit.asterisk.org/#/c/11337/7/tests/test_res_prometheus.c 
File tests/test_res_prometheus.c:

https://gerrit.asterisk.org/#/c/11337/7/tests/test_res_prometheus.c@1 
PS7, Line 1: /*
> The test leaks.  Ran it 10 times... […]
Ack



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/11337
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I891433a272c92fd11c705a2c36d65479a415ec42
Gerrit-Change-Number: 11337
Gerrit-PatchSet: 8
Gerrit-Owner: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Comment-Date: Tue, 21 May 2019 01:36:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: George Joseph <gjoseph at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190520/8a3db46d/attachment.html>