Skip to content
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

[FSSDK-10090] Refactor ODP integration #920

Merged
merged 24 commits into from
Apr 5, 2024
Merged

Conversation

raju-opti
Copy link
Contributor

@raju-opti raju-opti commented Apr 2, 2024

Summary

Things fixed:

  • ODPManager should not be running and scheduling timer if ODP is not integrated to the project (which causes memory leak if one sdk instance is created per request)
  • CreateUserContext should work even when called before the datafile is downloaded and should send the identify ODP events after datafile download completes
  • Other automatic odp events (vuid registration, client initialized) should also be sent after datafile is available and should not be dropped if batching is disabled.
  • ODP manager should distinguish between when odp config is still not loaded and when odp is not integrated at all
  • made ODP config immutable
  • ODP event manager should not schedule timers to flush events if event batching is disabled (ie batch size == 1)
  • other code quality improvements

Test plan

  • added new tests where applicable, and updated old tests

Issues

  • FSSDK-10090

@raju-opti raju-opti requested a review from a team as a code owner April 2, 2024 16:23
@raju-opti raju-opti marked this pull request as draft April 2, 2024 16:23
@raju-opti raju-opti changed the title Raju/odp refac [FSSDK-10090] Refactor ODP integration Apr 4, 2024
@raju-opti raju-opti marked this pull request as ready for review April 4, 2024 01:38
@raju-opti raju-opti requested a review from jaeopt April 4, 2024 01:39
@coveralls
Copy link

coveralls commented Apr 4, 2024

Coverage Status

coverage: 90.313% (-0.07%) from 90.381%
when pulling 117a4b5 on raju/odp_refac
into 53a2042 on master.

Copy link
Contributor

@mikechu-optimizely mikechu-optimizely left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was a long one phew. Thanks for putting all your thoughts in.

👇 are just some non-show stopping, thoughts/suggestions.

lib/core/odp/odp_config.ts Show resolved Hide resolved
lib/core/odp/odp_config.ts Outdated Show resolved Hide resolved
lib/core/odp/odp_event_api_manager.ts Show resolved Hide resolved
lib/core/odp/odp_event_manager.ts Show resolved Hide resolved
lib/core/odp/odp_event_manager.ts Outdated Show resolved Hide resolved
lib/core/odp/odp_event_manager.ts Show resolved Hide resolved
lib/core/odp/odp_manager.ts Show resolved Hide resolved
lib/optimizely/index.ts Show resolved Hide resolved
tests/odpManager.spec.ts Outdated Show resolved Hide resolved
Copy link

@muzahidul-opti muzahidul-opti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@mikechu-optimizely mikechu-optimizely left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One clarification - I see the updateOdpConfig on config change is broken. Can you take a look?

lib/core/odp/odp_event_manager.ts Outdated Show resolved Hide resolved
lib/core/odp/odp_event_manager.ts Outdated Show resolved Hide resolved
lib/core/odp/odp_event_manager.ts Show resolved Hide resolved
lib/core/odp/odp_event_manager.ts Show resolved Hide resolved
lib/optimizely/index.ts Show resolved Hide resolved
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mikechu-optimizely mikechu-optimizely merged commit 75a0148 into master Apr 5, 2024
15 of 19 checks passed
@mikechu-optimizely mikechu-optimizely deleted the raju/odp_refac branch April 5, 2024 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants