Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Prometheus metrics #184

Closed
wants to merge 8 commits into from
Closed

Add Prometheus metrics #184

wants to merge 8 commits into from

Conversation

thedevop
Copy link
Collaborator

@thedevop thedevop commented Mar 1, 2023

@mochi-co , I had to do go vendor to add modules in go.mod. So you may want to ignore go.mod and go.sum and regenerate them on your side.

PacketsSent: atomic.LoadInt64(&i.PacketsSent),
MemoryAlloc: atomic.LoadInt64(&i.MemoryAlloc),
Threads: atomic.LoadInt64(&i.Threads),
// InflightDropped: atomic.LoadInt64(&i.InflightDropped),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to be commented?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to mention, it appears InflightDropped is not used any more, I can't find any reference where the broker updates it.

{"c", "messages_dropped", "A counter of total number of publish messages dropped to slow subscriber", &i.MessagesDropped},
{"g", "retained", "A gauge of total number of retained messages active on the broker", &i.Retained},
{"g", "inflight", "A gauge of the number of messages currently in-flight", &i.Inflight},
// {"c", "inflight_dropped", "A", &i.InflightDropped},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above, InflightDropped is not updated


buildInfo := prometheus.NewGaugeVec(
prometheus.GaugeOpts{
// Namespace: AppName,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find any reference as to what's the app name you call this. mqtt, mochicomqtt, etc? can you provide one?

@@ -68,6 +69,9 @@ func (l *HTTPStats) Init(log *zerolog.Logger) error {

mux := http.NewServeMux()
mux.HandleFunc("/", l.jsonHandler)
mux.Handle("/metrics", promhttp.Handler())

// mux.Handle("/metrics", promhttp.)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that was a mistake... 😄

@mochi-co
Copy link
Collaborator

mochi-co commented Mar 3, 2023

Overall this looks good! However, I wonder if it would be possible to implement it as a hook, as not everyone will want to overhead of prometheus in the main code.

If we usedOnSysInfoTick, we just need a way to it to the http sys info listener. Which makes me wonder if that should be hook based too...

@thedevop
Copy link
Collaborator Author

thedevop commented Mar 4, 2023

I was originally thinking creating a sub-module, but since all its data are coming from the System, so it may make sense just be part of that.

The way I registered the metrics, there is really no activities on it unless there is a pull on the handle. And upon pull, it does atomic load of the values.

I was thinking of putting as a config into the sysinfo, but that config only contains TLS and is shared with many other listeners. As a result I hardcoded into the sysinfo listener. I'm open to suggestion how you would like to have this implemented:

  1. implement at server level, add config to server Options
  2. implement at sysinfo listener, use sysinfo specific config
  3. as a hook, but i'm not sure if it's appropriate, as it really is just a one time registration, and connect to a HTTP handler

@thedevop
Copy link
Collaborator Author

I'll resubmit a PR in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants