Closed marwan37 closed 4 months ago
[!IMPORTANT]
Auto Review Skipped
Auto reviews are disabled on this repository.
Please check the settings in the CodeRabbit UI or the
.coderabbit.yaml
file in this repository.To trigger a single review, invoke the
@coderabbitai review
command.
This update introduces a new configuration parameter max_retries
to the analytics module, initially set to 0. It also enhances the track_event
function with error handling to manage exceptions during analytics tracking, aiming to improve reliability and user experience by preventing the application from hanging due to unreachable analytics services.
File | Summary |
---|---|
.../analytics/client.py |
Added max_retries config parameter; updated track_event with try-except for exceptions |
max_retries
may also relate to the objectives of improving reliability and user experience by ensuring core operations are not affected by analytics failures.Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
Thank you for this PR! One thing you'll probably need to do is rebase on develop
branch. If your branch is branched off develop
already, then you can just click the 'edit' button at the right / top of the header and then update it so that it knows it'll be merged onto develop
and not to main
(which it's currently set to do).
Thanks!
You can also see the CI is showing a linting error, so please do run the formatting and linting scripts locally before updating with the fix.
Just fixed any issues mentioned in the CI and ran the format and lint scripts again. This was indeed branched off develop
already so the edit button at the top allowed me to update it. Thanks!
@coderabbitai review
Great suggestion, @strickvl. I've committed the change. Thanks for your input. Regarding the other CodeRabbit suggestion, I'm assuming it's safe to ignore as the app uses a single instance of the analytics client?
@marwan37 yep sometimes the rabbit has good suggestions or catches small things. Today, not so much :) You can ignore them.
@marwan37 sorry the line I suggested seems to have been too long. I'd break up that logger statement and then I'll rerun the CI.
@strickvl, I've adjusted the logger statement. Thanks for pointing that out.
@marwan37 I cloned your fork + checked out your branch but when running this I still get the following output when testing locally:
i.e. it doesn't seem to pick up your custom error at all. Looking at their codebase, it's not clear to me whether the on_error
is doing what we think it's doing actually... @bcdurak any thoughts? This e.g. is the place where the log I'm getting is generated.
@strickvl, yes I had those errors too. My apologies, I should have documented that behavior earlier. These errors seem to be handled internally by Segment and won't trigger the custom on_error
. Currently, it should be assumed that the on_error
handler is designed primarily for handling errors that occur after the retry logic has concluded. Without directly modifying the Segment library, I couldn't find a way to gain more control over those logs, but I'd be happy to explore further if needed.
@marwan37 can you run the format / linting script on the files on your end and push any changes that get made?
Describe changes
I implemented a workaround that disables retries for sending analytics data, in response to issue #130. This ensures the CLI remains responsive even when the Segment analytics API is unreachable, due to network failures or blocking mechanisms like PiHole.
src/mlstacks/analytics/client.py
to setmax_retries
to0
, which can be adjusted as needed.analytics_context.track
call in atry/except
block within thetrack_event
function. This ensures thereturn False
statement is reachable for error handling and logging exceptions.Reference to Documentation
The Segment Python library documentation did not explicitly mention configurable options for
max_retries
, but the source code revealed amax_retries
property insegment/analytics/client.py
.Testing
To simulate an unreachable analytics API domain at api.segment.io, I redirected all requests to this domain to 127.0.0.1 by adding the following entry in the /etc/hosts file on my Mac:
Following this setup, I tested the
deploy
,breakdown
,output
, anddestroy
CLI commands, and confirmed it exits immediately, without hanging, after attempting to reach the analytics API domain once.Pre-requisites
Please ensure you have done the following:
develop
and the open PR is targetingdevelop
. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes
Summary by CodeRabbit
max_retries
for enhanced control over analytics tracking.