search

asterisk / asterisk

The official Asterisk Project repository.
https://www.asterisk.org
Other
1.99k stars 929 forks source link

res_prometheus: Do not generate broken metrics #85

Closed zecke closed 10 months ago

zecke commented 1 year ago

In 8d6fdf9c3adede201f0ef026dab201b3a37b26b6 invisible bridges were skipped but that lead to producing metrics with no name and no help.

Keep track of the number of metrics configured and then only emit these. Add a basic testcase that verifies that there is no '(NULL)' in the output.

ASTERISK-30474

gtjoseph commented 1 year ago

If this PR applies to other branches, please add a comment with the appropriate "cherry-pick-to" headers as per the Create a Pull Request process. If you don't want it cherry-picked, please add a comment stating "No cherry-picks required" so we don't keep asking.

zecke commented 1 year ago

cherry-pick-to: 20 cherry-pick-to: 18

gtjoseph commented 1 year ago

I removed 19 from the cherry-pick branches as that branch is in security-fix-only mode.

seanbright commented 1 year ago

Also, can you update the commit message to reference 8d6fdf9c3adede201f0ef026dab201b3a37b26b6 rather than I58ef9f44036feded5966b5fc70ae754f8182883d since gerrit is no more?

zecke commented 11 months ago

@seanbright could you please have another look. I updated the commit message but I am not sure whether locking the bridge is the right thing.

jcolp commented 11 months ago

@seanbright could you please have another look. I updated the commit message but I am not sure whether locking the bridge is the right thing.

The bridge should not be locked when ast_bridge_destroy is called.

Additionally you've now got a second merge commit, this should be squashed down to a single commit on this PR.

zecke commented 11 months ago

@seanbright could you please have another look. I updated the commit message but I am not sure whether locking the bridge is the right thing.

The bridge should not be locked when ast_bridge_destroy is called.

Additionally you've now got a second merge commit, this should be squashed down to a single commit on this PR.

I ended up pressing the "Update branch" button in this UI. Rebased, forced push, reran the tests. Anything else that I should be doing?

jcolp commented 11 months ago

Nope.

zecke commented 11 months ago

Nope.

I tried to re-read https://wiki.asterisk.org/wiki/display/AST/Code+Contribution but I am not sure what to expect in terms of getting this merged. Anything I am missing?

jcolp commented 11 months ago

No, you're not missing anything. It requires someone else to do a review and then if approved would be merged in.

vinzens commented 10 months ago

Is there anybody else able to approve this PR? Would be great if Prometheus becomes usable.

jcolp commented 10 months ago

I've asked this to be looked at by someone else. Apparently it wasn't showing up for others.

github-actions[bot] commented 10 months ago

Successfully merged to branch master and cherry-picked to ["20","18"]