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

Use appropriate context to call NGINX Plus API #858

Open
lucacome opened this issue Sep 26, 2024 · 1 comment · May be fixed by #868
Open

Use appropriate context to call NGINX Plus API #858

lucacome opened this issue Sep 26, 2024 · 1 comment · May be fixed by #868
Milestone

Comments

@lucacome
Copy link
Member

https://github.com/nginxinc/nginx-plus-go-client/releases/tag/v2.0.0 now supports passing a context to the API calls, so we need to pass a context to c.nginxClient.GetStats()

func (c *NginxPlusCollector) Collect(ch chan<- prometheus.Metric) {
c.mutex.Lock() // To protect metrics from concurrent collects
defer c.mutex.Unlock()
stats, err := c.nginxClient.GetStats()

Figure out if we want to pass a context or a timeout. We already have a timeout that users can set for scraping metrics

timeout = createPositiveDurationFlag(kingpin.Flag("nginx.timeout", "A timeout for scraping metrics from NGINX or NGINX Plus.").Default("5s").Envar("TIMEOUT").HintOptions("5s", "10s", "30s", "1m", "5m"))

it probably makes sense to use the same value.

@lucacome lucacome added this to the v1.4.0 milestone Sep 26, 2024
@sjberman
Copy link

I think that makes sense to use as the same timeout.

@lucacome lucacome linked a pull request Oct 1, 2024 that will close this issue
6 tasks
@shaun-nx shaun-nx moved this from Todo ☑ to In Review 👀 in NGINX Ingress Controller Oct 9, 2024
@shaun-nx shaun-nx moved this from In Review 👀 to Todo ☑ in NGINX Ingress Controller Oct 9, 2024
@mpstefan mpstefan moved this from 🆕 New to ✅ Done in NGINX Gateway Fabric Oct 9, 2024
@AlexFenlon AlexFenlon moved this from Todo ☑ to In Review 👀 in NGINX Ingress Controller Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Status: In Review 👀
Development

Successfully merging a pull request may close this issue.

2 participants