Skip to content

Optimise startup time#169

Merged
NelsonVides merged 2 commits intomasterfrom
optimise_startup_time
Feb 26, 2025
Merged

Optimise startup time#169
NelsonVides merged 2 commits intomasterfrom
optimise_startup_time

Conversation

@NelsonVides
Copy link
Copy Markdown
Member

@NelsonVides NelsonVides commented Feb 25, 2025 •
edited
Loading

Fixes #81. Overrides #150.

Note too that this is already fixed in RabbitMQ for example, so it won't break its use-case: rabbitmq/rabbitmq-server@692b6f6

As in the last commit:

This means that by default prometheus won't try to find all loaded
modules implementing a behaviour, as this can be a very expensive
operation (taking many seconds as has been seen in the field). If such
behaviour was desired, it'd require explicitly setting all_loaded for
instrumeters and collectors. The default is the most sane behaviour.

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 25, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 96.66667% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/prometheus_misc.erl 91.66% 1 Missing ⚠️
Files with missing lines Coverage Δ
src/prometheus_collector.erl 97.14% <100.00%> (+0.08%) ⬆️
src/prometheus_instrumenter.erl 83.33% <100.00%> (-16.67%) ⬇️
test/eunit/prometheus_collector_tests.erl 100.00% <100.00%> (ø)
test/eunit/prometheus_instrumenter_tests.erl 100.00% <100.00%> (ø)
src/prometheus_misc.erl 86.66% <91.66%> (-7.78%) ⬇️

... and 2 files with indirect coverage changes

mikpe
mikpe reviewed Feb 25, 2025
View reviewed changes
Comment thread README.md
mikpe
mikpe reviewed Feb 25, 2025
View reviewed changes
Comment thread src/prometheus_collector.erl
mikpe
mikpe reviewed Feb 25, 2025
View reviewed changes
Comment thread README.md Outdated
mikpe
mikpe reviewed Feb 25, 2025
View reviewed changes
Comment thread src/prometheus_misc.erl Outdated
mikpe
mikpe previously approved these changes Feb 25, 2025
View reviewed changes
lhoguin
lhoguin previously approved these changes Feb 25, 2025
View reviewed changes
Copy link
Copy Markdown

@lhoguin lhoguin left a comment

Choose a reason for hiding this comment

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

Much better than the previous behavior.

lhoguin
lhoguin reviewed Feb 25, 2025
View reviewed changes
Comment thread src/prometheus_sup.erl Outdated
@NelsonVides NelsonVides force-pushed the optimise_startup_time branch from 448a146 to 0f56e41 Compare February 25, 2025 19:40
@NelsonVides NelsonVides dismissed stale reviews from lhoguin and mikpe via 0f56e41 February 25, 2025 19:45
@NelsonVides NelsonVides requested a review from mikpe February 25, 2025 19:45
mikpe
mikpe previously approved these changes Feb 26, 2025
View reviewed changes
@NelsonVides NelsonVides force-pushed the optimise_startup_time branch from 0f56e41 to 0a5c595 Compare February 26, 2025 09:15
@NelsonVides
Copy link
Copy Markdown
Member Author

NelsonVides commented Feb 26, 2025

@lhoguin @mikpe, just pushed a change, need another approval 🥲

@NelsonVides
break: Improve startup time of the prometheus application
fb4a374 
This means that by default prometheus won't try to find all loaded
modules implementing a behaviour, as this can be a very expensive
operation (taking many seconds as has been seen in the field). If such
behaviour was desired, it'd require explicitly setting `all_loaded` for
instrumeters and collectors. The default is the most sane behaviour.
@NelsonVides NelsonVides force-pushed the optimise_startup_time branch from 0a5c595 to fb4a374 Compare February 26, 2025 09:18
@NelsonVides NelsonVides merged commit f23df2d into master Feb 26, 2025
@NelsonVides NelsonVides deleted the optimise_startup_time branch February 26, 2025 14:25
@NelsonVides NelsonVides mentioned this pull request Feb 26, 2025
Closed
@albertored
Copy link
Copy Markdown

albertored commented Mar 19, 2025

Please note that this a breaking change.
Who is not explicitly defining collectors in application environment will see some metrics disappearing by simply updating the lib to a new minor version.

@NelsonVides
Copy link
Copy Markdown
Member Author

NelsonVides commented Mar 19, 2025

Please note that this a breaking change. Who is not explicitly defining collectors in application environment will see some metrics disappearing by simply updating the lib to a new minor version.

Fair enough. So you mean this should be released as a 5.0? Can fix if that's the case.

@albertored
Copy link
Copy Markdown

albertored commented Mar 19, 2025

Ideally yes, otherwise it can be explicitly stated in the documentation and/or changelog.
It is a tricky change because if you don't have tests on all your collectors (and on the ones included from libraries) you may not discover the issue before going to production

@NelsonVides
Copy link
Copy Markdown
Member Author

NelsonVides commented Mar 19, 2025

Ideally yes, otherwise it can be explicitly stated in the documentation and/or changelog. It is a tricky change because if you don't have tests on all your collectors (and on the ones included from libraries) you may not discover the issue before going to production

Done, released 5.0.0 and retired 4.13.0 👍🏽

Thanks for pointing that out!

@albertored
Copy link
Copy Markdown

albertored commented Mar 19, 2025

@NelsonVides can you also release a version of prometheus_ex that allows to use the new 5.0.0?

@NelsonVides
Copy link
Copy Markdown
Member Author

NelsonVides commented Mar 19, 2025

@NelsonVides can you also release a version of prometheus_ex that allows to use the new 5.0.0?

@albertored would love to, but actually that repo needs a revamp, like creating new CI with all the latest versions of Erlang/Elixir, a new ex_doc, fixing the configs (Mix.Config is deprecated), etc. Maybe you want to contribute a PR cleaning up that one? Would be very appreciated! 😄

@albertored
Copy link
Copy Markdown

albertored commented Mar 19, 2025

@NelsonVides prometheus-erl/prometheus.ex#57

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mikpe mikpe mikpe approved these changes

@lhoguin lhoguin lhoguin approved these changes

@deadtrickster deadtrickster Awaiting requested review from deadtrickster

@mkuratczyk mkuratczyk Awaiting requested review from mkuratczyk

@onno-vos-dev onno-vos-dev Awaiting requested review from onno-vos-dev

@DenysGonchar DenysGonchar Awaiting requested review from DenysGonchar

@MirahImage MirahImage Awaiting requested review from MirahImage

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Slow Supervisor Boot Time

4 participants

@NelsonVides @albertored @mikpe @lhoguin