-
Notifications
You must be signed in to change notification settings - Fork 85
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
feat: Add LDConfig.sendEvents
option to disable all events
#414
Conversation
aa8586c
to
fcb8599
Compare
@@ -60,7 +60,7 @@ final class DarklyService: DarklyServiceProvider { | |||
self.context = context | |||
self.serviceFactory = serviceFactory | |||
|
|||
if !config.mobileKey.isEmpty && !config.diagnosticOptOut { | |||
if !config.mobileKey.isEmpty && !config.diagnosticOptOut && config.sendEvents { |
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 change alone is sufficient to disable diagnostic events. If the cache doesn't exist, we won't collect diagnostic events.
func makeDiagnosticReporter(service: DarklyServiceProvider, environmentReporter: EnvironmentReporting) -> DiagnosticReporting { | ||
DiagnosticReporter(service: service, environmentReporting: environmentReporter) | ||
func makeDiagnosticReporter(config: LDConfig, service: DarklyServiceProvider, environmentReporter: EnvironmentReporting) -> DiagnosticReporting { | ||
if config.sendEvents && !config.diagnosticOptOut { |
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 we also add this bit to make it a little more efficient by not making a reporter 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 suspense between the two comments. I read the first one and I was like, "yes, but that indirectness scares me".
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, I hate that there are even two paths like this. But there are protocols that are depending on others in a cycle and so really teasing that out as much as I would prefer is probably more effort than it is worth. Especially since we're pushing for a larger re-work.
🤖 I have created a release *beep* *boop* --- ## [9.12.0](9.11.0...9.12.0) (2024-11-06) ### Features * Add `LDConfig.sendEvents` option to disable all events ([#414](#414)) ([9a51844](9a51844)) * Add cache usage option for identify calls ([#408](#408)) ([b928345](b928345)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: LaunchDarklyReleaseBot <LaunchDarklyReleaseBot@launchdarkly.com>
No description provided.