Conversation
✅ Deploy Preview for polite-licorice-3db33c ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
dosubot
bot
added
the
size:XL
Summary of ChangesHello @toddbaert, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a new Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new rollout operator for gradual feature flag rollouts over time. The implementation is well-structured and includes thorough tests covering determinism, time progression, and integration. My review focuses on improving robustness and correctness. I've suggested using int64 for timestamps to avoid future overflow issues, adding a necessary check for evaluation context properties, and cleaning up the sample file which contains outdated and incorrect examples. I also included minor suggestions to improve logging practices.
| startTime int32 | ||
| endTime int32 | ||
| currentTime int32 |
There was a problem hiding this comment.
Using int32 for Unix timestamps is risky due to the Year 2038 problem, as they will overflow. Please use int64 for startTime, endTime, and currentTime to ensure the long-term correctness of this feature. This will also require updating extractInt to handle int64 and adjusting the type conversions in parseRolloutData.
| startTime int32 | |
| endTime int32 | |
| currentTime int32 | |
| startTime int64 | |
| endTime int64 | |
| currentTime int64 |
| properties, _ := getFlagdProperties(dataMap) | ||
| config := &rolloutConfig{ | ||
| currentTime: int32(properties.Timestamp), | ||
| data: data, | ||
| } |
There was a problem hiding this comment.
The boolean return value from getFlagdProperties is being ignored. This could lead to unexpected behavior if the $flagd properties are missing or malformed in the evaluation context. It's safer to check this value and return an error if it's false.
properties, ok := getFlagdProperties(dataMap)
if !ok {
return nil, errors.New("$flagd properties missing or invalid in context")
}
config := &rolloutConfig{
currentTime: int32(properties.Timestamp),
data: data,
}| func (r *Rollout) Evaluate(values, data any) any { | ||
| config, err := r.parseRolloutData(values, data) | ||
| if err != nil { | ||
| r.Logger.Warn(fmt.Sprintf("parse rollout evaluation data: %v", err)) |
There was a problem hiding this comment.
Instead of using fmt.Sprintf to format the error into the log message, it's better to use structured logging by passing the error as a separate field (e.g., zap.Error(err)). This improves log parsing and filtering. You will need to add an import for go.uber.org/zap.
| r.Logger.Warn(fmt.Sprintf("parse rollout evaluation data: %v", err)) | |
| r.Logger.Warn("parse rollout evaluation data", zap.Error(err)) |
core/pkg/evaluator/rollout.go
Outdated
| // evaluate nested JSONLogic | ||
| result, err := jsonlogic.ApplyInterface(v, data) | ||
| if err != nil { | ||
| r.Logger.Warn(fmt.Sprintf("error evaluating nested JSONLogic in rollout: %v", err)) |
There was a problem hiding this comment.
Instead of using fmt.Sprintf to format the error into the log message, it's better to use structured logging by passing the error as a separate field (e.g., zap.Error(err)). This improves log parsing and filtering. You will need to add an import for go.uber.org/zap.
r.Logger.Warn("error evaluating nested JSONLogic in rollout", zap.Error(err))
toddbaert
force-pushed
the
feat/rollout
branch
from
072d869 to
5095462
Compare
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
toddbaert
force-pushed
the
feat/rollout
branch
from
5095462 to
7c08c99
Compare
|
1 participant
Draft implementation of a
rolloutoperator.For a demo with virtual users in your console, do:
For full details and justification, please see ADR: #1867