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

discovery: add metrics + send updates from one goroutine only #4667

Merged

Conversation

simonpasquier
Copy link
Member

The added metrics are:

  • prometheus_sd_discovered_targets
  • prometheus_sd_received_updates_total
  • prometheus_sd_updates_delayed_total
  • prometheus_sd_updates_total

cc @krasi-georgiev

The added metrics are:

* prometheus_sd_discovered_targets
* prometheus_sd_received_updates_total
* prometheus_sd_updates_delayed_total
* prometheus_sd_updates_total

Signed-off-by: Simon Pasquier <[email protected]>
@simonpasquier simonpasquier requested review from krasi-georgiev and removed request for krasi-georgiev September 27, 2018 14:04
Copy link
Contributor

@krasi-georgiev krasi-georgiev left a comment

Choose a reason for hiding this comment

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

LGTM with just few rename suggestions

thanks for simplifying!

syncCh chan map[string][]*targetgroup.Group

// How long to wait before sending updates to the channel. The variable
// should only be modified in unit tests.
updatert time.Duration

// The trigger channel signals to the manager that new updates have been received from providers.
trigger chan struct{}
Copy link
Contributor

Choose a reason for hiding this comment

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

how about triggerSend

}
}

func (m *Manager) sendUpdates() {
Copy link
Contributor

Choose a reason for hiding this comment

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

how about just sender() ?

Signed-off-by: Simon Pasquier <[email protected]>
@simonpasquier
Copy link
Member Author

@krasi-georgiev done!

@simonpasquier
Copy link
Member Author

/benchmark

@prombot
Copy link
Contributor

prombot commented Sep 28, 2018

@simonpasquier: Welcome to Prometheus Benchmarking Tool.

The two prometheus versions that will be compared are pr-4667 and master

The logs can be viewed at the links provided in the GitHub check blocks at the end of this conversation

After successfull deployment, the benchmarking metrics can be viewed at :

To stop the benchmark process comment /benchmark cancel .

In response to this:

/benchmark

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@simonpasquier
Copy link
Member Author

/benchmark cancel

@prombot prombot removed the benchmark label Oct 1, 2018
@simonpasquier
Copy link
Member Author

The benchmark doesn't show significant differences on resource usage before and after this PR but if master had the prometheus_sd_updates_total metric, we would probably have seen a decrease of the metric's value with the PR.

@krasi-georgiev
Copy link
Contributor

even if it doesn't improve performance it is easier to read so I think it still an improvement.

@krasi-georgiev
Copy link
Contributor

LGTM

@simonpasquier simonpasquier merged commit f033f48 into prometheus:master Oct 4, 2018
@simonpasquier simonpasquier deleted the add-discovery-metrics branch October 4, 2018 13:30
@krasi-georgiev
Copy link
Contributor

don't forget to squash on merge as otherwise we end up with non build-able commits.

the merge button has a drop-down so you can select it by default.

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.

3 participants