-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[exporterhelper] Fix exporter.PersistRequestContext feature gate #13359
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
[exporterhelper] Fix exporter.PersistRequestContext feature gate #13359
Conversation
d6423ab to
7a16214
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #13359 +/- ##
==========================================
+ Coverage 91.52% 91.56% +0.04%
==========================================
Files 528 528
Lines 29478 29478
==========================================
+ Hits 26980 26992 +12
+ Misses 1969 1960 -9
+ Partials 529 526 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| PersistRequestContextOnRead = func() bool { return PersistRequestContextFeatureGate.IsEnabled() } | ||
| PersistRequestContextOnWrite = func() bool { return PersistRequestContextFeatureGate.IsEnabled() } |
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.
Couldn't this be:
| PersistRequestContextOnRead = func() bool { return PersistRequestContextFeatureGate.IsEnabled() } | |
| PersistRequestContextOnWrite = func() bool { return PersistRequestContextFeatureGate.IsEnabled() } | |
| PersistRequestContextOnRead = PersistRequestContextFeatureGate.IsEnabled | |
| PersistRequestContextOnWrite = PersistRequestContextFeatureGate.IsEnabled |
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.
It can, but I would suggest we just remove the 2 temp vars and use the feature gate all the time.
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.
It can, but I would suggest we just remove the 2 temp vars and use the feature gate all the time.
Yeah, but otherwise it's very complicated to test the cases when the feature gate is changed while there is data in the persistent store
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.
Sure, but you need only the happy path to be tested in the e2e with real ptrace. For the enable/disable you can have unit-tests directly in the persistent_queue_test file and have this featuregate passed in the constructor (as hidden property) to the queue.
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 try this as a follow-up
It was ignored before because the PersistRequestContextOnRead and PersistRequestContextOnWrite variables were initialized before the feature gate was set from the command line arguments.
7a16214 to
8af6b02
Compare
af0fffd
It was ignored before because the PersistRequestContextOnRead and PersistRequestContextOnWrite variables were initialized before the feature gate was set from the command line arguments.
Resolves #13342