Skip to content

Conversation

@idoqo
Copy link
Contributor

@idoqo idoqo commented Nov 1, 2025

PMM-12478

Link to the Feature Build: SUBMODULES-0

If this PR adds, removes or alters one or more API endpoints, please review and add or update the relevant API documentation as well:

  • API Docs updated

If this PR is related to some other PRs in this or other repositories, please provide links to those PRs:

  • Links to related pull requests (optional).

@codecov
Copy link

codecov bot commented Nov 1, 2025

Codecov Report

❌ Patch coverage is 5.64516% with 117 lines in your changes missing coverage. Please review.
✅ Project coverage is 45.62%. Comparing base (586a922) to head (5b499d9).

Files with missing lines Patch % Lines
...anaged/services/agents/external_exporter_status.go 0.00% 65 Missing ⚠️
managed/cmd/pmm-managed/main.go 0.00% 37 Missing ⚠️
managed/services/management/external.go 0.00% 11 Missing ⚠️
managed/services/converters.go 55.55% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##               v3    #4726      +/-   ##
==========================================
- Coverage   45.75%   45.62%   -0.13%     
==========================================
  Files         364      365       +1     
  Lines       37755    37848      +93     
==========================================
- Hits        17275    17269       -6     
- Misses      18823    18920      +97     
- Partials     1657     1659       +2     
Flag Coverage Δ
admin 17.92% <ø> (ø)
agent 53.58% <ø> (-0.13%) ⬇️
managed 46.07% <5.64%> (-0.15%) ⬇️
vmproxy 72.09% <ø> (ø)

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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

map<string, string> custom_labels = 7;
// Group name of external service.
string group = 8;
// Access address (DNS name or IP).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Access address (DNS name or IP).
// Service address (DNS name or IP).

string group = 8;
// Access address (DNS name or IP).
string address = 9;
// Access port.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Access port.
// Service port.


s.l.Debugf("Updating status for %d external exporters.", len(statusMap))

err = s.db.InTransaction(func(tx *reform.TX) error {
Copy link
Member

Choose a reason for hiding this comment

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

Better use .InTransactionContext wherever the context is available.

status = inventoryv1.AgentStatus_AGENT_STATUS_UNKNOWN
}

err = s.db.InTransaction(func(tx *reform.TX) error {
Copy link
Member

Choose a reason for hiding this comment

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

.InTransactionContext?

// This allows VictoriaMetrics to scrape the exporter and generate 'up' metric
agentID := agent.AgentId
go func() {
time.Sleep(15 * time.Second)
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a magic number or a named constant?

// This allows VictoriaMetrics to scrape the exporter and generate 'up' metric
agentID := agent.AgentId
go func() {
time.Sleep(15 * time.Second)
Copy link
Member

@ademidoff ademidoff Nov 6, 2025

Choose a reason for hiding this comment

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

IMO everything should be context-bound and the sleep itself should be cancellable.

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.

4 participants