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

Include global metrics in the per-request collector registry #956

Closed
wants to merge 1 commit into from

Conversation

candlerb
Copy link
Contributor

This is a quick proof-of-concept fix for the zero metrics (snmp_packet_duration_seconds_bucket/sum/count, snmp_unexpected_pdu_type_total, snmp_packets_total, snmp_packet_retries_total) and missing metrics (snmp_collection_duration_seconds_count/sum, snmp_request_errors_total)

To consider:

  • Do we really want the NxM metrics for snmp_collection_duration_seconds_count/sum (for every possible combination of auth and module, even those not being used?) Maybe this can be dropped given that Support fetching multiple modules in one scrape #945 will be introducing per-module scrape metrics?
  • The code could be simplified a little by removing the metrics member from the Collector struct, and using globalMetrics directly wherever it's needed, instead of passing it around.

Fixes #950

@candlerb candlerb force-pushed the candlerb/950 branch 3 times, most recently from a2b1d10 to 99fa3dd Compare August 17, 2023 15:26
@candlerb
Copy link
Contributor Author

Aside: for some reason I get E-mail notifications of "golangci-lint workflow run" failing when I push to my own repo:

View workflow run

lint: generator/config.go#L1
: # github.com/prometheus/snmp_exporter/generator

lint: generator/net_snmp.go#L19
fatal error: net-snmp/net-snmp-config.h: No such file or directory

lint
issues found

However, the checks in the pull request pass.

@SuperQ
Copy link
Member

SuperQ commented Aug 27, 2023

Hmm, I'm not sure about this approach. I much prefer using promauto and passing a registry.

If we need to init some metrics, it's better to loop over things with metric.WithLabelValues(...).

As for the labels, I'm on the fence.

@candlerb
Copy link
Contributor Author

Hmm, I'm not sure about this approach. I much prefer using promauto and passing a registry.

I think a global persistent registry makes a lot of sense, but I don't know how to do that given that the current architecture creates a disposable registry for every request, containing a dynamically-created collector:

func handler(w http.ResponseWriter, r *http.Request, logger log.Logger) {
        ...
        registry := prometheus.NewRegistry()
        c := collector.New(r.Context(), target, authName, auth, nmodules, logger, registry, *concurrency)
        registry.MustRegister(c)
        // Delegate http serving to Prometheus client library, which will call collector.Collect.
        h := promhttp.HandlerFor(registry, promhttp.HandlerOpts{})
        h.ServeHTTP(w, r)
}

Perhaps collector.Collect() could delegate to the global registry, is that what you mean? (Can a registry register another registry? I haven't checked this)

Aside: I wondered briefly if this is a use case for Unregister but ruled it out, since there could be concurrent scrapes with different auth and module parameters.

@SuperQ
Copy link
Member

SuperQ commented Aug 28, 2023

I figured it out, see #971.

@candlerb
Copy link
Contributor Author

Being fixed in #971

@candlerb candlerb closed this Aug 28, 2023
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.

snmp_packet stats are all zeros
2 participants