flagsmith-charts icon indicating copy to clipboard operation
flagsmith-charts copied to clipboard

Deprecate InfluxDB

Open khvn26 opened this issue 2 years ago • 2 comments

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

  • [x] I have filled in the "Changes" section below?
  • [x] I have filled in the "How did you test this code" section below?
  • [x] 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 avatar Apr 19 '23 11:04 khvn26

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.

plumdog avatar Apr 19 '23 12:04 plumdog

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 https://github.com/Flagsmith/flagsmith-charts/pull/133/commits/142e2321491cbb52d9762464a0222bfa42d90914 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 avatar Apr 19 '23 14:04 khvn26