Skip to content

PMM-14141 #1107

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

PMM-14141 #1107

wants to merge 3 commits into from

Conversation

impimp
Copy link

@impimp impimp commented Jul 3, 2025

PMM-14141 (optional, if ticket reported)

  • Links to related pull requests (optional).

Below we provide a basic checklist of things that would make it a good PR:

  • Make sure to sign the CLA (Contributor License Agreement).
  • Make sure all tests pass.
  • Keep current with the target branch and fix conflicts if necessary.
  • Update jira ticket description if necessary.
  • Attach screenshots and/or console output to the jira ticket to confirm new behavior, if applicable.
  • Leave notes to the reviewers if you need to focus their attention on something specific.

Once all checks pass and the code is ready for review, please add pmm-review-exporters team as the reviewer. That would assign people from the review team automatically. Report any issues on our Forum.

@impimp impimp requested a review from a team as a code owner July 3, 2025 19:19
@impimp impimp requested review from BupycHuk and JiriCtvrtka and removed request for a team July 3, 2025 19:19
@it-percona-cla
Copy link

it-percona-cla commented Jul 3, 2025

CLA assistant check
All committers have signed the CLA.

Copy link

codecov bot commented Jul 3, 2025

Codecov Report

Attention: Patch coverage is 47.05882% with 9 lines in your changes missing coverage. Please review.

Project coverage is 65.29%. Comparing base (dc46ed5) to head (ee2070b).
Report is 111 commits behind head on main.

Files with missing lines Patch % Lines
exporter/exporter.go 47.05% 7 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1107      +/-   ##
==========================================
- Coverage   70.88%   65.29%   -5.60%     
==========================================
  Files          28       29       +1     
  Lines        3569     3080     -489     
==========================================
- Hits         2530     2011     -519     
- Misses        904      929      +25     
- Partials      135      140       +5     
Flag Coverage Δ
agent 65.29% <47.05%> (-5.60%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@BupycHuk
Copy link
Member

BupycHuk commented Jul 3, 2025

Hi, can you please address linter comments?

impimp added 2 commits July 4, 2025 08:50
The previous implementation expected this header to be an integer, but vmagent
 may send it as a float. This caused incorrect handling in `mongodb_exporter`.

Updated to support float values.
The exporter was returning cached MongoDB connections without validating their health. This meant that if a connection had become unhealthy,
subsequent operations would fail after a delay (serverSelectionTimeout etc), causing the entire request to be slower than it should be.

This commit adds  `Ping` check before returning a cached client.
@impimp
Copy link
Author

impimp commented Jul 4, 2025

Hi, can you please address linter comments?

Done.

Co-authored-by: Alex Demidoff <[email protected]>
@BupycHuk BupycHuk requested a review from Copilot July 7, 2025 09:10
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances the MongoDB client reconnection logic and refines the Prometheus scrape timeout parsing in the HTTP handler.

  • Adds a Ping check before reusing an existing MongoDB client in getClient.
  • Switches timeout header parsing from Atoi to ParseFloat and logs invalid values.
  • Updates the WithTimeout calculation to work with floating‐point seconds and subtracts the configured offset.
Comments suppressed due to low confidence (1)

exporter/exporter.go:309

  • [nitpick] The variable name seconds is generic. Renaming it to timeoutSeconds or similar would improve clarity.
		seconds := 10.0

@impimp impimp requested a review from ademidoff July 8, 2025 06:50
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.

6 participants