Skip to content

Add support for callbacks on config change #912

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

Merged
merged 15 commits into from
Sep 29, 2020

Conversation

basepi
Copy link
Contributor

@basepi basepi commented Sep 1, 2020

What does this pull request do?

Adds the ability to specify one or more "callback" functions which will be called when a _ConfigValue is changed.

Related issues

closes #908

@apmmachine
Copy link
Contributor

apmmachine commented Sep 1, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #912 updated]

  • Start Time: 2020-09-29T19:44:25.570+0000

  • Duration: 24 min 35 sec

Test stats 🧪

Test Results
Failed 0
Passed 10966
Skipped 8506
Total 19472

Steps errors

Expand to view the steps failures

  • Name: Shell Script
    • Description: ./tests/scripts/docker/run_tests.sh pypy-2 none

    • Duration: 3 min 9 sec

    • Start Time: 2020-09-29T19:50:31.978+0000

    • log

Copy link
Contributor

@beniwohli beniwohli left a comment

Choose a reason for hiding this comment

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

Looks like a clean solution!

I was wondering if it would be possible to use validators for the callback functionality, but this is much better.

Also added a _dict_key_lookup to _ConfigBase to allow for
easy access to _ConfigValue instances via dict_key
@basepi
Copy link
Contributor Author

basepi commented Sep 17, 2020

@beniwohli Something I realized while trying to figure out how to test this today -- callbacks will also be called on initial Config creation, for any non-default values. I think this is probably ok and expected behavior? But it's something to note, and I wondered if you saw any issues with that. I feel like any operation you want to perform on config change probably needs to be performed on initialization as well, if the config value is different than the default value, but I'm not certain.

@basepi basepi marked this pull request as ready for review September 17, 2020 22:08
@basepi basepi requested a review from beniwohli September 17, 2020 22:08
Copy link
Contributor

@beniwohli beniwohli left a comment

Choose a reason for hiding this comment

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

Awesome work! Thanks for taking the time to write those docs!

@basepi basepi merged commit 47574b4 into elastic:master Sep 29, 2020
romulorosa pushed a commit to romulorosa/apm-agent-python that referenced this pull request Oct 15, 2020
* Add support for callbacks on config value change

* Fix VersionedConfig.reset()

Also added a _dict_key_lookup to _ConfigBase to allow for
easy access to _ConfigValue instances via dict_key

* Remove unused variable, and use compat.iteritems

* Use default values for old_value comparison in callbacks

* Add remote config callback test

* Add to changelog

* Workaround for no nonlocal keyword in py2

* Add nonlocal note

* Add a reset() callback test, plus a typo fix
romulorosa pushed a commit to romulorosa/apm-agent-python that referenced this pull request Oct 15, 2020
* Add support for callbacks on config value change

* Fix VersionedConfig.reset()

Also added a _dict_key_lookup to _ConfigBase to allow for
easy access to _ConfigValue instances via dict_key

* Remove unused variable, and use compat.iteritems

* Use default values for old_value comparison in callbacks

* Add remote config callback test

* Add to changelog

* Workaround for no nonlocal keyword in py2

* Add nonlocal note

* Add a reset() callback test, plus a typo fix
beniwohli pushed a commit to beniwohli/apm-agent-python that referenced this pull request Sep 14, 2021
* Add support for callbacks on config value change

* Fix VersionedConfig.reset()

Also added a _dict_key_lookup to _ConfigBase to allow for
easy access to _ConfigValue instances via dict_key

* Remove unused variable, and use compat.iteritems

* Use default values for old_value comparison in callbacks

* Add remote config callback test

* Add to changelog

* Workaround for no nonlocal keyword in py2

* Add nonlocal note

* Add a reset() callback test, plus a typo fix
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.

Remote Config: Add callbacks on config change
4 participants