Skip to content

Stop formula engine #1165

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Feb 24, 2025
Merged

Stop formula engine #1165

merged 12 commits into from
Feb 24, 2025

Conversation

@ela-kotulska-frequenz
Copy link
Contributor

@ela-kotulska-frequenz ela-kotulska-frequenz commented Feb 18, 2025 •
edited
Loading

This PR introduces 4 new changes:

  • make formula stop method public
  • Add stop method to MetricFetcher - which stops receivers and FallbackMetricFetcher
  • Fix bug that raised many exception when formulas stopped
  • We can stop formula, now. So we can stop FallbackMetricFetcher when primary metric fetcher is good again.

All this simplified the code a lot.

@ela-kotulska-frequenz ela-kotulska-frequenz added priority:high Address this as soon as possible type:enhancement New feature or enhancement visitble to users labels Feb 18, 2025
@ela-kotulska-frequenz ela-kotulska-frequenz self-assigned this Feb 18, 2025
@ela-kotulska-frequenz ela-kotulska-frequenz requested a review from a team as a code owner February 18, 2025 10:54
@ela-kotulska-frequenz ela-kotulska-frequenz requested review from llucax and removed request for a team February 18, 2025 10:54
@github-actions github-actions bot added part:tests Affects the unit, integration and performance (benchmarks) tests part:data-pipeline Affects the data pipeline labels Feb 18, 2025
llucax
llucax reviewed Feb 18, 2025
View reviewed changes
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

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

LGTM but the description about the close issue should be removed from the OR description so it doesn't go to the merge commit and it needs release notes as the formula engine is public.

@ela-kotulska-frequenz ela-kotulska-frequenz marked this pull request as draft February 18, 2025 13:04
@ela-kotulska-frequenz ela-kotulska-frequenz force-pushed the stop_formula_engine branch 2 times, most recently from 23953ad to 0040a42 Compare February 18, 2025 15:20
@ela-kotulska-frequenz
Copy link
Contributor Author

One more error described in PR message

llucax
llucax reviewed Feb 20, 2025
View reviewed changes
@ela-kotulska-frequenz ela-kotulska-frequenz force-pushed the stop_formula_engine branch 2 times, most recently from a35dcfb to 57e01f1 Compare February 20, 2025 11:35
@github-actions github-actions bot added the part:docs Affects the documentation label Feb 20, 2025
@ela-kotulska-frequenz ela-kotulska-frequenz marked this pull request as ready for review February 20, 2025 11:36
@ela-kotulska-frequenz
Copy link
Contributor Author

Last commit is very complicated, sorry for that!
But those things are really related and one implied the other and I couldn't separate it.

@ela-kotulska-frequenz
Make stop method of FormulaEngine public
b38c420 
To be able to stop custom formulas outside sdk.

Signed-off-by: Elzbieta Kotulska <[email protected]>
@ela-kotulska-frequenz
Write debug instead of exception after FormulaEngine stop
200a398 
Replace exception with debug message to avoid confusing traceback output.
Previously, FormulaEngine couldn't be stopped, so exception was acceptable.
Now that FormulaEngine can be stopped, use severity level 'debug' since
stopping is a normal operation and usefull only during debugging.

Signed-off-by: Elzbieta Kotulska <[email protected]>
@ela-kotulska-frequenz ela-kotulska-frequenz force-pushed the stop_formula_engine branch 2 times, most recently from 133d022 to 0ff71bf Compare February 20, 2025 11:46
llucax
llucax reviewed Feb 20, 2025
View reviewed changes
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

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

I'll leave the changes in formula engine to @shsms as he is more familiar with that code base.

@ela-kotulska-frequenz
Refactor fetching from fallback_fetcher
29d1699 
Also simplify failure log message

Signed-off-by: Elzbieta Kotulska <[email protected]>
@ela-kotulska-frequenz
Rename primary_fetcher_sample → primary_fetcher_value
254095c 
Also allow `None` values for it.

Signed-off-by: Elzbieta Kotulska <[email protected]>
@ela-kotulska-frequenz
Refactor _fetch_next to prioritize the primary stream
5c0eed3 
When the primary stream has invalid data, then look at the fallback
stream.

This approach doesn't assume that once a fallback is started, it is
always running.

In a subsequent commit, we will implement stopping of fallback
fetchers when the primary has recovered.

Signed-off-by: Elzbieta Kotulska <[email protected]>
@ela-kotulska-frequenz
Remove unused fetch_next_with_fallback method
ad88541 
Signed-off-by: Elzbieta Kotulska <[email protected]>
@ela-kotulska-frequenz
Rename next_value → primary_value
03e90d8 
Signed-off-by: Elzbieta Kotulska <[email protected]>
@ela-kotulska-frequenz
Implement a stop method on MetricFetcher
7f3dcb8 
And use it to stop the fallback fetcher when it is no longer needed.

Signed-off-by: Elzbieta Kotulska <[email protected]>
@ela-kotulska-frequenz
Use a flag to identify if a ReceiverStoppedError is expected
1ba3043 
If it happens because the fetcher is being stopped, then only log a
DEBUG message.

Signed-off-by: Elzbieta Kotulska <[email protected]>
@ela-kotulska-frequenz
Update release notes
c9ebc4f 
Signed-off-by: Elzbieta Kotulska <[email protected]>
@ela-kotulska-frequenz
Add extra log about stop of fallback metric fetcher
91080f3 
Signed-off-by: Elzbieta Kotulska <[email protected]>
@ela-kotulska-frequenz
Replace catching ReceiverError[Any] with just ReceiverError
f21b112 
It looks like ReceiverError[Any] is not ReceiverError, but some
type object. When any error occured we got missleading
error about the exception not inheriting from BaseException.

Signed-off-by: Elzbieta Kotulska <[email protected]>
@ela-kotulska-frequenz
Copy link
Contributor Author

Thanks @shsms for separating these commits. Is much more readable now! :)

@ela-kotulska-frequenz ela-kotulska-frequenz added this pull request to the merge queue Feb 24, 2025
Merged via the queue into frequenz-floss:v1.x.x with commit 3a3071f Feb 24, 2025
5 checks passed
@ela-kotulska-frequenz ela-kotulska-frequenz deleted the stop_formula_engine branch February 24, 2025 10:33
@llucax llucax added this to the v1.0.0-rc1700 milestone Feb 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shsms shsms shsms approved these changes

@llucax llucax Awaiting requested review from llucax

Assignees

@ela-kotulska-frequenz ela-kotulska-frequenz

Labels

part:data-pipeline Affects the data pipeline part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests priority:high Address this as soon as possible type:enhancement New feature or enhancement visitble to users

Projects

Python SDK Roadmap
Status: Done

Milestone

v1.0.0-rc1700

Development

Successfully merging this pull request may close these issues.

3 participants

@ela-kotulska-frequenz @llucax @shsms