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 timeout flag for context cancelation #868

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lucacome
Copy link
Member

@lucacome lucacome commented Oct 1, 2024

Proposed changes

This pull request introduces several changes to the NGINX Prometheus exporter to add context and timeout handling capabilities. The main updates include modifying the GetStubStats method to accept a context parameter, adding a timeout field to the NginxCollector and NginxPlusCollector structs, and updating the Collect methods to use context with timeout.

Context and Timeout Handling Enhancements:

  • client/nginx.go: Modified the GetStubStats method to accept a context.Context parameter, enabling more flexible request handling.

  • collector/nginx.go:

    • Added context and time imports to support context with timeout.
    • Added a timeout field to the NginxCollector struct and updated the NewNginxCollector function to accept a timeout parameter.
    • Updated the Collect method to use a context with timeout for fetching stub stats.
  • collector/nginx_plus.go:

    • Added time import to support context with timeout.
    • Added a timeout field to the NginxPlusCollector struct and updated the NewNginxPlusCollector function to accept a timeout parameter. [1] [2] [3] [4]
    • Updated the Collect method to use a context with timeout for fetching NGINX Plus stats.

Code Cleanup:

  • exporter.go:
    • Simplified the registerCollector function signature and removed the Timeout field from the http.Client configuration. [1] [2] [3]

Closes #858

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING guide
  • I have proven my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have ensured the README is up to date
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch on my own fork

@lucacome lucacome requested a review from a team as a code owner October 1, 2024 02:51
@lucacome lucacome self-assigned this Oct 1, 2024
@github-actions github-actions bot added the enhancement Pull requests for new features/feature enhancements label Oct 1, 2024
Copy link

codecov bot commented Oct 1, 2024

Codecov Report

Attention: Patch coverage is 0% with 14 lines in your changes missing coverage. Please review.

Project coverage is 3.89%. Comparing base (9cd3941) to head (487aac4).

Files with missing lines Patch % Lines
collector/nginx.go 0.00% 5 Missing ⚠️
collector/nginx_plus.go 0.00% 5 Missing ⚠️
exporter.go 0.00% 3 Missing ⚠️
client/nginx.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##            main    #868      +/-   ##
========================================
- Coverage   3.90%   3.89%   -0.01%     
========================================
  Files          5       5              
  Lines       1307    1310       +3     
========================================
  Hits          51      51              
- Misses      1256    1259       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mutex sync.Mutex
}

// NewNginxCollector creates an NginxCollector.
func NewNginxCollector(nginxClient *client.NginxClient, namespace string, constLabels map[string]string, logger *slog.Logger) *NginxCollector {
func NewNginxCollector(nginxClient *client.NginxClient, namespace string, constLabels map[string]string, logger *slog.Logger, timeout time.Duration) *NginxCollector {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this function provide a sensible default for the timeout? Currently it breaks public API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Pull requests for new features/feature enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use appropriate context to call NGINX Plus API
3 participants