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

Deprecate InfluxDB #133

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Deprecate InfluxDB #133

wants to merge 3 commits into from

Conversation

khvn26
Copy link
Member

@khvn26 khvn26 commented Apr 19, 2023

Thanks for submitting a PR! Please check the boxes below:

  • I have filled in the "Changes" section below?
  • I have filled in the "How did you test this code" section below?
  • I have bumped the version number in /charts/flagsmith/Chart.yaml in the section version or I'm merging to a
    release branch

Changes

This PR deprecates InfluxDB usage:

  • InfluxDB is disabled by default.
  • InfluxDB is not provisioned by the chart anymore, influxdb2 dependency removed.
  • PostgreSQL backend is used for analytics.

How did you test this code?

  1. Installed the chart with Helm 3.11.3 to a k8s v1.26.3 cluster (minikube).
  2. Created an organization, project and a feature. Evaluated the flag with an SDK.
  3. Verified that analytics get inserted in PostgreSQL.

@khvn26 khvn26 requested a review from plumdog April 19, 2023 11:40
@plumdog
Copy link
Contributor

plumdog commented Apr 19, 2023

I'm definitely in favour of this change as it simplifies the deployment requirements. Has the application deprecated InfluxDB support? If so, is there an intended date or release version when support will be dropped from the application?

As for the changes in the chart, I think there are 3 different kinds of customer to think about:

  • Customers not using analytics at all at present
    • for these customers, this change will mean they gain analytics but backed by Postgres. Might there be reasons a customer wouldn't want this? Eg due to storage or performance impact of storing analytics? Does this need to be communicated?
  • Customers using analytics using the in-chart InfluxDB
    • this change would mean that their InfluxDB would removed, and their data would be lost (strictly, it might be recoverable, depending how the underlying PV is configured, I suppose). I think this probably needs some communication or warning perhaps, or some guidance about how to save that data (or perhaps just making clear it is not supported).
    • also worth noting, that this is the default configuration, so I think there will be plenty of customers with this setup
  • Customers using analytics with a separately managed InfluxDB
    • I think they will be unaffected by this change
    • Though if there's a future application release that will remove InfluxDB support, that would need to be communicated

An alternative approach that might require less proactive customer communication would be to:

  • make a release where InfluxDB defaults to enabled, but the application defaults to not using it, and helm prints a warning at deployment time. Could even show a message within the web application to appropriate users, though this would need an application change.
    • so anyone deploying this version without realising what it contained wouldn't lose their data, but analytics would stop working, and they'd see a warning and realise what was happening. The warning should information about InfluxDB deprecation and what to do. The application could be configured to continue using InfluxDB for analytics by setting useDeprecatedInfluxDbForAnalytics: true or something in the chart values. This could also make clear when InfluxDB support in the chart would be completely removed.
    • this release could also contain something to copy the analytics data from InfluxDB into Postgres if this is possible
  • then make a future release that does remove InfluxDB
    • this would be released after the previously stated date when InfluxDB support would be removed.

@khvn26
Copy link
Member Author

khvn26 commented Apr 19, 2023

Hi @plumdog and thanks for the very valuable feedback!

The PR essentially solves #126 – Flagsmith is still, and will be, able to leverage InfluxDB backend for analytics, but we opt for dropping the InfluxDB deployment from our charts, providing a minimal working configuration based on the existing PostgreSQL database.

I suspected the drop was too abrupt. I suppose I can only leave 142e232 and add the following modifications:

  • influxdb2.enabled will be set to true by default, as before.
  • There'll be no option to use influxdb2 as the analytics backend, only PostgreSQL and external InfluxDB.
  • The application will be configured with analytics turned off. A new flag will be responsible for this.
  • A deprecation message will be added to NOTES.txt to explain the transition.

Does this look good to you?

@khvn26 khvn26 force-pushed the improvement/deprecate-influx branch from 3b54e53 to b167961 Compare April 20, 2023 10:39
@khvn26 khvn26 mentioned this pull request Apr 20, 2023
3 tasks
@khvn26 khvn26 force-pushed the improvement/deprecate-influx branch 5 times, most recently from ef37319 to edf9c8f Compare April 20, 2023 21:39
- name: Package the pre-release chart
run: helm package ./charts/flagsmith --version ${{ steps.get-version.outputs.version }} --destination out

- name: Save the pre-release chart archive
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reasoning here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Trying to automate branch-based chart packaging so I can fiddle with Terraform with less hoops to jump through. Doesn't work as intended, I removed this.

##### Chart-provisioned InfluxDB is deprecated. #####
######################################################

If you were provisioning an InfluxDB instance for Flagsmith's
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know that we need to worry about this but, just so I have the full picture... is there a way that users will be able to configure the charts such that their chart-provisioned influxdb will continue to be used? Also, I guess this throws the question about migration. We haven't written anything to migrate from InfluxDB to the Postgres backed analytics solution. The experience will actually be that they appear to lose all of the data they had in Influx once the API starts returning data from the postgres DB rather than Influx. So, you're right that the data isn't 'lost' but it will appear to be from the users' perspectives.

Copy link
Member Author

@khvn26 khvn26 Apr 21, 2023

Choose a reason for hiding this comment

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

is there a way that users will be able to configure the charts such that their chart-provisioned influxdb will continue to be used?

I imagine two paths for this:

  1. Deploying an external InfluxDB instance and migrating the data.
  2. Configuring the ealier chart-provisioned instance as an external one (perhaps we should test and document this).

We haven't written anything to migrate from InfluxDB to the Postgres backed analytics solution.

True. No idea how hard would that be. I'm not sure there'd be high demand for this too, given that InfluxDB is still usable.

@khvn26 khvn26 force-pushed the improvement/deprecate-influx branch 3 times, most recently from bb29e28 to 38dea2e Compare April 26, 2023 10:42
@khvn26 khvn26 requested a review from a team as a code owner May 15, 2024 05:57
@khvn26 khvn26 requested review from gagantrivedi and removed request for a team and plumdog May 15, 2024 05:57
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.

None yet

4 participants