Updated to new Azure ServiceBus Go SDK #6
Conversation
|
Hi Anders, thank you for the contribution. Unfortunately I won't be able to take a look at this for next ~1,5 weeks. I'll try to come back to this ASAP. |
|
No worries, I realized after I'd done the upgrade that I needed some custom logic in the exporter, so I wrote a new one for my specific use case. But I'll still follow up on this one if there are anything you would like me to change/fix for the PR. |
| @@ -5,8 +5,9 @@ import ( | |||
| "net/http" | |||
| "time" | |||
|
|
|||
| "github.com/namsral/flag" | |||
| "github.com/sirupsen/logrus" | |||
| "flag" | |||
There was a problem hiding this comment.
This change removed support for passing configuration via environment variables
| "github.com/sirupsen/logrus" | ||
| "flag" | ||
|
|
||
| klog "k8s.io/klog/v2" |
There was a problem hiding this comment.
What is the reason to switch to klog?
| type config struct { | ||
| timeout time.Duration | ||
| port uint | ||
| verbose bool |
There was a problem hiding this comment.
can you explain removal of the verbose flag?
| } | ||
|
|
||
| log.WithFields(logrus.Fields{ |
There was a problem hiding this comment.
can you explain removal of this diagnostic log?
| @@ -23,6 +25,7 @@ type MessageCounts struct { | |||
| ScheduledMessages int32 | |||
| TransferDeadLetterMessages int32 | |||
| TransferMessages int32 | |||
| TotalMessageCount int64 | |||
There was a problem hiding this comment.
This value is not collected in collectMessageCounts in collector.go
| @@ -50,6 +50,8 @@ func newMessageCountMetrics(itemName string, labels ...string) messageCountMetri | |||
| fmt.Sprintf("%s transfer messages count", itemName), labels, nil), | |||
| transferDeadLetterMessages: prometheus.NewDesc(fmt.Sprintf("servicebus_%s_transfer_dead_letter_messages", strings.ToLower(itemName)), | |||
| fmt.Sprintf("%s transfer dead letter messages count", itemName), labels, nil), | |||
| totalMessageCount: prometheus.NewDesc(fmt.Sprintf("servicebus_%s_transfer_total_messages", strings.ToLower(itemName)), | |||
There was a problem hiding this comment.
this one should be servicebus_%s_total_messages I think?
Changes:
Only thing is the new TotalMessages field on Topic Subscriptions. We could potentially put that in the existing ScheduledMessages field there, not sure about that one.