feat: support running without a STATSIG_API_KEY #6403
Conversation
| // dummy key as fallback for e2e tests, which use local mode | ||
| 'client-key' | ||
| export const STATSIG_API_KEY = keyOrUndefined(secretsFile, DEFAULT_TESTNET, 'STATSIG_API_KEY') | ||
| export const STATSIG_ENABLED = !isE2EEnv && !!STATSIG_API_KEY |
There was a problem hiding this comment.
Kept isE2EEnv here as when running locally, the secrets are decrypted, but we should still keep statsig disabled.
I think with the move to the runtime we'll update "secrets" management and will just use env variables. So this special case of e2e env will not be needed anymore.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6403 +/- ##
==========================================
+ Coverage 89.09% 89.12% +0.02%
==========================================
Files 736 737 +1
Lines 31840 31897 +57
Branches 6040 5750 -290
==========================================
+ Hits 28367 28427 +60
+ Misses 3425 3422 -3
Partials 48 48
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
src/analytics/AppAnalytics.ts
Outdated
| // StableID should match Segment anonymousId | ||
| overrideStableID, | ||
| environment: STATSIG_ENV, | ||
| localMode: isE2EEnv, |
There was a problem hiding this comment.
Should we just initialize the statsig client in local mode if statsig is disabled? That way we can avoid the custom gate overrides code too. And we can avoid the noisy logs by tweaking the logging condition.
Also, it seems like for statsig to be enabled, we'll need the segment key too because of the stable id dependency. Wonder if we should remove that dependency. We can probably let statsig set it's own anonymous id in cases where segment is not enabled.
There was a problem hiding this comment.
ah I didn't know about the localMode option.
However my intention is to be able to run even if the statsig lib is not installed. So the approach here of not instantiating the client is preferrable I think. We're planning to make 3rd party deps like segment, clevertap, etc optional in the modular runtime.
There was a problem hiding this comment.
Also, it seems like for statsig to be enabled, we'll need the segment key too because of the stable id dependency. Wonder if we should remove that dependency. We can probably let statsig set it's own anonymous id in cases where segment is not enabled.
Yes I thought about that, and was on the fence whether to make that change now.
I'm not sure if we'll need statsig enabled without segment.
To run experiments we need segment to be connected to statsig, right?
There was a problem hiding this comment.
makes sense re running without installing those libraries.
For experiments, yes segment is needed but it doesn't need to be. Metrics can potentially go to statsig directly without segment. Also, maybe there's a case where statsig is just needed for dynamic config. But I guess we could do that separately.
should we just drop the localMode option now that it is always going to be false?
There was a problem hiding this comment.
yes definitely I'll remove it.
src/analytics/AppAnalytics.ts
Outdated
| // StableID should match Segment anonymousId | ||
| overrideStableID, | ||
| environment: STATSIG_ENV, | ||
| localMode: isE2EEnv, |
There was a problem hiding this comment.
makes sense re running without installing those libraries.
For experiments, yes segment is needed but it doesn't need to be. Metrics can potentially go to statsig directly without segment. Also, maybe there's a case where statsig is just needed for dynamic config. But I guess we could do that separately.
should we just drop the localMode option now that it is always going to be false?
Description
As the title says.
Before this, running the app without a STATSIG_API_KEY was throwing a bunch of errors. And crashing in dev mode in the settings.
See mobilestack-xyz#2 (comment)
Test plan
Related issues
Backwards compatibility
Yes
Network scalability
If a new NetworkId and/or Network are added in the future, the changes in this PR will: