-
Notifications
You must be signed in to change notification settings - Fork 670
Add new 'type' and 'encryption' labels to weave_connections metric #3789
Add new 'type' and 'encryption' labels to weave_connections metric #3789
Conversation
Hi, if you're not finding time to come back to this I'll pick it up. |
@bboreham thanks - I had this on my ever-growing TODO list, but simply haven't gotten back to it yet... if you're able to handle this that would be much appreciated! 😅 |
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
We know all the expected states, so can pre-create maps
Extend tests to check it shows up. Tests assume that metrics are published with metrics in alphabetical order, which appears to be the case for now.
I added the test, and also the branch needed rebasing so I did that, and then added an @hairyhenderson I can try to push that to your branch if you like, or I can merge from my branch. |
@bboreham thanks! (and sorry again for the slow response time) Your changes LGTM, and please feel free to push to this branch - I've enabled the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smoke-tests won't run when the branch is pushed from a 3rd-party repo, but they did complete at the same commit here:
https://circleci.com/gh/weaveworks/weave/tree/add-connection-type-label-to-weave-connection-metric-3788
Thanks so much @bboreham - and sorry again for dropping the ball on this one 😅 |
Fixes #3788
Notes:
make
succeeds, so that's encouraging...state
label's value to the values inallConnectionStates
, the state will reflect whatever's inconn.State
(which may be out of sync withallConnectionStates
some day? 🤷♂)name
attribute was a non-string, but not sure if this is really necessary. Maybe removing the check and allowing a panic instead would be better somehow?Signed-off-by: Dave Henderson dhenderson@gmail.com