-
Notifications
You must be signed in to change notification settings - Fork 398
[FFL-1319] Add feature flags events exposure #5024
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
|
Thank you for updating Change log entry section 👏 Visited at: 2025-11-11 09:56:41 UTC |
BenchmarksBenchmark execution time: 2025-11-12 21:17:16 Comparing candidate commit ad31051 in PR branch Found 2 performance improvements and 0 performance regressions! Performance is the same for 42 metrics, 2 unstable metrics. scenario:profiling - stack collector (native frames - native filenames disabled)
scenario:profiling - stack collector (native frames - native filenames enabled)
|
dbc899c to
0a25c37
Compare
b1a978f to
9e7efb0
Compare
Typing analysisNote: Ignored files are excluded from the next sections. Untyped methodsThis PR introduces 2 untyped methods and 9 partially typed methods. It increases the percentage of typed methods from 53.12% to 54.63% (+1.51%). Untyped methods (+2-0)❌ Introduced:Partially typed methods (+9-0)❌ Introduced:Untyped other declarationsThis PR introduces 1 untyped other declaration and 4 partially typed other declarations. It increases the percentage of typed other declarations from 66.92% to 67.95% (+1.03%). Untyped other declarations (+1-0)❌ Introduced:Partially typed other declarations (+4-0)❌ Introduced:If you believe a method or an attribute is rightfully untyped or partially typed, you can add |
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🔗 Commit SHA: d5cd2ee | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
d7424f9 to
2221c34
Compare
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.
Left a few notes!
| module Datadog | ||
| module OpenFeature | ||
| module Exposures | ||
| BufferBaseClass = | ||
| (Core::Environment::Ext::RUBY_ENGINE == 'ruby') ? Core::Buffer::CRuby : Core::Buffer::ThreadSafe | ||
|
|
||
| class Buffer < BufferBaseClass |
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.
Minor: From what I understood, this feature will require libdatadog and thus not work on JRuby. So... should we raise if we're not on CRuby instead?
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 didn't know that libdatadog is not working on JRuby, noted 👍🏼
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.
@ivoanjo Do you think I just can use CRuby as we will not announce the JRuby support on that feature.
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! Maybe keep the branch and set it to nil on JRuby? Or :unsupported? Just so that if we ever want to support JRuby, the code "reminds" us to update itself.
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.
(Also on libdatadog + JRuby it indeed doesn't work at the moment and I have not seen any plans to change that)
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.
Not sure that I get what do you want to keep, like base class definition with :unsupported instead of the class name?
Co-authored-by: Ivo Anjo <[email protected]>
cdf5175 to
e1171ed
Compare
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 gave a pass on everything other than sig/ and spec/ and ran out of time for this! Hopefully this helps.
In particular make sure to review my note on the components around the shutdown so we don't leak any threads
Co-authored-by: Sameeran Kunche <[email protected]>
Co-authored-by: Ivo Anjo <[email protected]>
Co-authored-by: Ivo Anjo <[email protected]>
Co-authored-by: Ivo Anjo <[email protected]>
Co-authored-by: Ivo Anjo <[email protected]>
Co-authored-by: Ivo Anjo <[email protected]>
Co-authored-by: Ivo Anjo <[email protected]>
Co-authored-by: Ivo Anjo <[email protected]>
Co-authored-by: Ivo Anjo <[email protected]>
| end | ||
|
|
||
| def duplicate?(event) | ||
| cache_key = digest(event.flag_key, event.targeting_key) |
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.
Since duplicate? is called from the critical path, do we need to digest and encode the keys in crc32; or just quickly check their uniqueness?
If we just need a quick check, we probably use [event.flag_key, event.targeting_key] as @cache keys directly.
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.
Good question, I can store the strings, but I want to save some memory as I don't control the length of the values, but I know the length of the crc32.
I will post a benchmark.
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.
benchmark.rb
# frozen_string_literal: true
require 'benchmark/ips'
require 'zlib'
A = 'something'
B = 'someting_else'
AA = 'something' * 10
BB = 'someting_else' * 10
GC.disable
def concatenate(a, b)
"#{a}:#{b}"
end
def concatenate_with_crc32(a, b)
Zlib.crc32("#{a}:#{b}")
end
Benchmark.ips do |x|
x.report('concatenate') { concatenate(A, B) }
x.report('concatenate+crc32') { concatenate_with_crc32(A, B) }
x.report('concatenate+long') { concatenate(AA, BB) }
x.report('concatenate+long+crc32') { concatenate_with_crc32(AA, BB) }
x.compare!
end
ruby 3.3.5 (2024-09-03 revision ef084cc8f4) [arm64-darwin23]
Warming up --------------------------------------
concatenate 605.455k i/100ms
concatenate+crc32 784.806k i/100ms
concatenate+long 97.743k i/100ms
concatenate+long+crc32
88.121k i/100ms
Calculating -------------------------------------
concatenate 6.086M (±83.0%) i/s (164.31 ns/i) - 16.953M in 5.300050s
concatenate+crc32 5.865M (±44.5%) i/s (170.49 ns/i) - 20.405M in 5.302478s
concatenate+long 8.762M (±66.2%) i/s (114.13 ns/i) - 6.353M in 5.115903s
concatenate+long+crc32
6.117M (±48.7%) i/s (163.48 ns/i) - 8.195M in 5.235183s
Comparison:
concatenate+long: 8761760.7 i/s
concatenate+long+crc32: 6116833.6 i/s - same-ish: difference falls within error
concatenate: 6086228.7 i/s - same-ish: difference falls within error
concatenate+crc32: 5865284.7 i/s - same-ish: difference falls within error
ruby 3.2.2 (2023-03-30 revision e51014f9c0) [arm64-darwin23]
Warming up --------------------------------------
concatenate 842.935k i/100ms
concatenate+crc32 714.773k i/100ms
concatenate+long 147.856k i/100ms
concatenate+long+crc32
82.161k i/100ms
Calculating -------------------------------------
concatenate 4.796M (±69.4%) i/s (208.50 ns/i) - 18.545M in 5.270077s
concatenate+crc32 5.203M (±41.4%) i/s (192.18 ns/i) - 20.728M in 5.067348s
concatenate+long 8.305M (±67.6%) i/s (120.41 ns/i) - 6.358M in 5.325213s
concatenate+long+crc32
6.280M (±47.7%) i/s (159.23 ns/i) - 8.052M in 5.191753s
Comparison:
concatenate+long: 8305246.8 i/s
concatenate+long+crc32: 6280105.7 i/s - same-ish: difference falls within error
concatenate+crc32: 5203346.6 i/s - same-ish: difference falls within error
concatenate: 4796064.6 i/s - same-ish: difference falls within error
ruby 3.0.7p220 (2024-04-23 revision 724a071175) [arm64-darwin24]
Warming up --------------------------------------
concatenate 1.112M i/100ms
concatenate+crc32 737.016k i/100ms
concatenate+long 684.174k i/100ms
concatenate+long+crc32
549.157k i/100ms
Calculating -------------------------------------
concatenate 11.205M (± 6.3%) i/s (89.25 ns/i) - 56.710M in 5.081533s
concatenate+crc32 7.331M (± 1.2%) i/s (136.41 ns/i) - 36.851M in 5.027715s
concatenate+long 5.398M (±36.9%) i/s (185.25 ns/i) - 23.946M in 5.175464s
concatenate+long+crc32
5.122M (±14.8%) i/s (195.24 ns/i) - 25.261M in 5.163119s
Comparison:
concatenate: 11205066.3 i/s
concatenate+crc32: 7330601.8 i/s - 1.53x slower
concatenate+long: 5398187.4 i/s - 2.08x slower
concatenate+long+crc32: 5121816.8 i/s - 2.19x slower
So on Ruby 3.0 it will be slower, but rest is fine
| event = Event.build(result, flag_key: flag_key, context: context) | ||
| return false if @deduplicator.duplicate?(event) |
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.
Most events will be built just to be discarded. I see in Event.build that it creates quite a few Ruby objects.
Do you think it's ok to check the duplicator before building the event?
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 can sacrifice ergonomics for performance here
| ) | ||
|
|
||
| if result.error? | ||
| return ::OpenFeature::SDK::Provider::ResolutionDetails.new( |
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.
Any reason why we use the upstream ResolutionDetails here, but our own copy of ResolutionDetails in other places?
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.
Yep, unfortunately, this is an agreed way, even tho we can duck-type here, we must return the SDK "type" (sorry)
What does this PR do?
Adds new component under
lib/datadog/open_feature/folder that is providing customer an ability to configure Datadog feature flags provider. This provider is going to rely on Remote Configuration to deliver feature flags configurations (aka UFC Universal Flag Configuration).Said provider is going into customer code as a part of configuration for OpenFeature
Motivation:
This is a part of upcoming work across all libs.
Important
This code doesn't contain actual evaluation logic, but rather establish everything for it to be placed in the next PR.
Change log entry
Yes. OpenFeature: Add experimental OpenFeature component.
Additional Notes:
This PR was already reviewed as a part of #5022, but accidentally was marked as ready for review. This code is fresh and already applied some suggestions from the previous review, but not everything is possible due to the existing established way or the time boundaries.
Structure:
I would like to ask some pair of eyes on thread-safety and potential issues of new component. I've run the code over certain checks already, but never enough.
How to test the change?
CI and we will have new set of ST in #5022 enabled to prove that everything works.