-
Notifications
You must be signed in to change notification settings - Fork 399
FFL-1273 Native feature flags evaluation #5054
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: master
Are you sure you want to change the base?
Conversation
|
👋 Hey @p-datadog, please fill "Change log entry" section in the pull request description. If changes need to be present in CHANGELOG.md you can state it this way **Change log entry**
Yes. A brief summary to be placed into the CHANGELOG.md(possible answers Yes/Yep/Yeah) Or you can opt out like that **Change log entry**
None.(possible answers No/Nope/None) Visited at: 2025-11-18 22:25:28 UTC |
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage 🔗 Commit SHA: 1ae3feb | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
90dd290 to
df67799
Compare
71e5459 to
76ac04c
Compare
BenchmarksBenchmark execution time: 2025-11-27 17:42:16 Comparing candidate commit 1ae3feb in PR branch Found 1 performance improvements and 1 performance regressions! Performance is the same for 42 metrics, 2 unstable metrics. scenario:profiling - intern mixed existing and new
scenario:profiling - intern_all 1000 repeated strings
|
76ac04c to
ccef8cc
Compare
Typing analysisNote: Ignored files are excluded from the next sections. Untyped methodsThis PR introduces 3 untyped methods and 1 partially typed method. It increases the percentage of typed methods from 54.99% to 55.21% (+0.22%). Untyped methods (+3-0)❌ Introduced:Partially typed methods (+1-0)❌ Introduced:If you believe a method or an attribute is rightfully untyped or partially typed, you can add |
ccef8cc to
a6db269
Compare
This commit is recovering Sameeran's work from #5034
a6db269 to
106a374
Compare
Co-authored-by: Leo Romanovsky <[email protected]>
106a374 to
4862383
Compare
5effc95 to
8e202f6
Compare
|
@codex review |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Strech
left a comment
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.
Left a super-small correction of comment. I think we are good to go as soon as we get 👍🏼 from the perf team
ivoanjo
left a comment
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'm a bit short on time today so I gave a pass only on the feature_flags.c file -- that part LGTM (after a final suggested fix)
y9v
left a comment
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.
left some non-blocking comments
3171bd5 to
74a8452
Compare
74a8452 to
262cd28
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
devflow unqueued this merge request: It did not become mergeable within the expected time |
|
|
||
| // Forward declarations | ||
| static VALUE configuration_new(VALUE klass, VALUE json_str); | ||
| static void configuration_free(void *ptr); |
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 this function taking a configuration as a parameter? If yes configuration would be a better name for the argument, void * already indicates it's a pointer.
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 and no. ptr is a data portion of RTypedData. It happens to be ddog_ffe_Handle_Configuration right now, so it's technically a configuration but not the one exposed to Ruby.
Note that this function is not exposed to Ruby, it's a part of integration with GC
| VALUE feature_flags_module = rb_define_module_under(core_module, "FeatureFlags"); | ||
|
|
||
| rb_gc_register_address(&feature_flags_error_class); | ||
| feature_flags_error_class = rb_define_class_under(feature_flags_module, "Error", rb_eStandardError); |
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 class is also defined in Ruby code, what is the purpose of having two definitions?
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.
We need one here so we can refer to it later. I think we can remove the one defined in Ruby, although it would make the code confusing.
cc @Strech for your opinion
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.
(For what it's worth, I like keeping the definitions on the Ruby side too, as a lot of tools gets confusing with the Ruby -> C transition and so it's clearer to have something on the Ruby side even though it gets used from C.)
(Instead of defining a class, it's possible to read from the constant instead, but the amount of code is similar so I'd keep it as-is)
|
@p-datadog I'll work on addressing the comments but could you please clarify what comments are actually blocking the merge? |
| break; | ||
| default: | ||
| // Skip unsupported attribute types. | ||
| return ST_CONTINUE; |
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 there an exception guard around the entire feature? If so, would it make sense to raise an exception here instead of silently ignoring unhandled input?
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, there is a guard. No, it doesn't make sense because we can continue and produce results in most cases.
Co-authored-by: Oleg Pudeyev <[email protected]>
* Add comments about reconfiguration argument and methods
What does this PR do?
Implement OpenFeature evaluator using bindings to native libdatadog evaluator.
Change log entry
Additional Notes:
How to test the change?