-
Notifications
You must be signed in to change notification settings - Fork 185
Initial work on managed variables #1548
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
base: main
Are you sure you want to change the base?
Conversation
|
|
||
|
|
||
| @dataclass(init=False) | ||
| class VariablesOptions: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having both VariablesOptions and VariablesConfig is worrying
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I'm assuming you understand the difference between them, do you have a suggestion for alternative naming?
|
@alexmojaki I've opened a new pull request, #1549, to work on those changes. Once the pull request is ready, I'll request review from you. |
Deploying logfire-docs with
|
| Latest commit: |
5d3e20b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://39882494.logfire-docs.pages.dev |
| Branch Preview URL: | https://managed-variables.logfire-docs.pages.dev |
logfire/variables/remote.py
Outdated
| # TODO: Ideally we'd be able to terminate while the following request was going even if it takes a while | ||
| # Note it's far more reasonable to terminate this worker thread "gracelessly" than an OTel exporter's. | ||
| # Is there anything similar to an anyio CancelScope we can use here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't see any reason to try and quickly cancel things in _worker. it's a daemon thread so it won't hold up process shutdown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'm concluding that the point is we don't feel a need to actually join the thread at all? The main issue I see with that is potential issues in pytest with resources not getting cleaned up properly but it might be fine, I'll try to not do any joining and we can revisit this if it causes problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Note to self: my main fear was that this would cause an issue with testing.)
735b295 to
ce01469
Compare
| # ============================================================================= | ||
| # Test Condition Classes | ||
| # ============================================================================= | ||
|
|
||
|
|
||
| class TestValueEquals: | ||
| def test_matches_when_equal(self): | ||
| condition = ValueEquals(attribute='plan', value='enterprise') | ||
| assert condition.matches({'plan': 'enterprise'}) is True | ||
|
|
||
| def test_no_match_when_different(self): | ||
| condition = ValueEquals(attribute='plan', value='enterprise') | ||
| assert condition.matches({'plan': 'free'}) is False | ||
|
|
||
| def test_no_match_when_missing(self): | ||
| condition = ValueEquals(attribute='plan', value='enterprise') | ||
| assert condition.matches({}) is False | ||
|
|
||
| def test_kind_discriminator(self): | ||
| condition = ValueEquals(attribute='plan', value='enterprise') | ||
| assert condition.kind == 'value-equals' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reads like the LLM knows it's being paid by the token
| VariantKey = str | ||
| VariableName = str | ||
|
|
||
| # TODO: Do we need to make the following dataclasses into pydantic dataclasses or BaseModels so the validators run when |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, why not?
| assert len(override.conditions) == 1 | ||
| assert override.rollout.variants == {'premium': 1.0} | ||
|
|
||
| def test_multiple_conditions(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs to test behaviour
logfire/variables/config.py
Outdated
| Rollout schedules progress through stages sequentially, with each stage having its own | ||
| duration, rollout configuration, and optional conditional overrides. This allows for | ||
| gradual rollouts where traffic percentages can increase over time. | ||
| Example: A three-stage rollout might have: | ||
| - Stage 1: 5% of traffic for 1 hour (canary) | ||
| - Stage 2: 25% of traffic for 4 hours (early adopters) | ||
| - Stage 3: 100% of traffic (full rollout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i like this example better than the one in RolloutSchedule. but it makes sense for all the explanations of a schedule to be in RolloutSchedule, not here.
i don't like the idea of having to define many stages if i want a gradual rollout e.g. increase by 5% every hour. i'd rather say "increase linearly from x% to y% starting at time T over N hours".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but do you really mind if that happens in UI rather than in python struct? Doing it in UI means we can have a clean configuration option but can change the logic etc. and not require ambitious changes, doing it in struct means we need to support all the potential patterns we'd like for shorthand configuration. Happy to discuss more but I'm not convinced that a linear rollout is right most of the time, or that end users (in aggregate) won't end up needing flexibility.
logfire/variables/config.py
Outdated
| # Note: we could add this client side using the logfire query client if the token has read capability. | ||
| # However, this should maybe be discouraged if it's viable to run health check queries server-side. | ||
| # We could expose a `health_check` field that contains one (or more?) SQL queries, which would either be | ||
| # evaluated client side or server side. However, I don't love the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I think I've convinced myself that whole set of behavior can live entirely server side / in CRUD for us, and you'd use IaC if you wanted to manage it in code. And that IaC config doesn't need to live here. Maybe that would be the right way to handle all of this schedule stuff, I think we can remove it from the PR for now.
logfire/variables/config.py
Outdated
| """Duration to remain in this stage before progressing to the next. | ||
| Once a stage's duration has elapsed, the schedule automatically advances to the | ||
| next stage. If this is the final stage and its duration has elapsed, the schedule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so does the final stage need a duration at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the point was that there is a meaning to the value even if it's not currently used. We can remove/comment-out all this code for now, probably makes sense to discuss how we want to handle this synchronously.
logfire/variables/config.py
Outdated
| start_at: datetime | None | ||
| """The datetime when this schedule becomes active. | ||
| If None, the schedule is inactive and the base rollout is used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mean None or having a way to specify the start time? It's useful to specify the start time — e.g. if you want the feature to go live tomorrow morning once the team is ready to deal with issues.
I added the ability to set it to None as a way to store the schedule on the config before you've decided precisely when you want it to go live. You could achieve this by just setting it to some time far in the future but I was also thinking this could play nice with automated rollback — if there's a failure, we update the config to have no start time here (and maybe have some other error field populated). I'm 100% open to tweaks to this.
logfire/variables/config.py
Outdated
| next stage. If this is the final stage and its duration has elapsed, the schedule | ||
| is considered complete. | ||
| Note: Automated rollback based on error rates is only supported server-side and should |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
automated rollback of any kind sounds like quite an ambitious thing to implement. why are we even implementing scheduled stages right now at all? can we just get something basic out first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I explained this to you elsewhere, but the point was I wanted to convince myself that we weren't going to feel the need to add/modify the config shape to support these features if/when we wanted to ship them in the future. I'm comfortable removing and/or commenting out fields/code related to this for now.
…t so pydantic-core doesn't get built
No description provided.