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

Track topics used by a Team on Team Broker #4748

Merged
merged 17 commits into from
Nov 19, 2024
Merged

Track topics used by a Team on Team Broker #4748

merged 17 commits into from
Nov 19, 2024

Conversation

hardillb
Copy link
Contributor

@hardillb hardillb commented Nov 9, 2024

part of #4726

Description

Tracks all topics published to on the Team Broker.

Adds 1 new HTTP endpoint

  • GET /api/v1/teams/:teamId/broker/topics returns an array of strings representing the topics the team has published to

Requires team Member or Owner to access list of topics

Needs:

  • testing
  • a new permission adding to access the endpoint

Currently caches topics for 1 hour, checks for removing topics every 30seconds, probably should be configurable.

Will benefit from using Redis for this so cache is preserved over restarts and if multiple instances of forge app running.

Related Issue(s)

#4726

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

  • Includes a DB migration? -> add the area:migration label

Copy link

codecov bot commented Nov 9, 2024

Codecov Report

Attention: Patch coverage is 59.09091% with 18 lines in your changes missing coverage. Please review.

Project coverage is 78.73%. Comparing base (9c1babb) to head (3e5816d).
Report is 38 commits behind head on main.

Files with missing lines Patch % Lines
forge/ee/lib/teamBroker/index.js 55.88% 15 Missing ⚠️
forge/housekeeper/tasks/teamBroker.js 25.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4748      +/-   ##
==========================================
- Coverage   78.79%   78.73%   -0.06%     
==========================================
  Files         311      312       +1     
  Lines       14787    14831      +44     
  Branches     3387     3395       +8     
==========================================
+ Hits        11651    11677      +26     
- Misses       3136     3154      +18     
Flag Coverage Δ
backend 78.73% <59.09%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Copy link
Member

@knolleary knolleary left a comment

Choose a reason for hiding this comment

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

Inspired thinking - I like it.

I suggest structuring topic list to have teamId has a top level key, rather than just a flat list of all topics.

{
   "ff/v1/team1" : {
      "foo": { ttl: 123 }
   },
   "ff/v1/team2" : {
      "bar": { ttl: 123 }
   }
}

That will make the get operation more scalable as it doesn't have to iterate over the entire cache.

@hardillb
Copy link
Contributor Author

hardillb commented Nov 11, 2024

I did it this way to make it easier to include the project nodes changes later (when they will publish without the mount point so will need to include the full topic prefix).

Might need to discuss/think about it some more.

@hardillb
Copy link
Contributor Author

Not sure how to write a test for those last 5 lines as that would mean adding a 30 second pause to ensure the interval runs (but even then it won't remove any entries for 30mins)

@joepavitt
Copy link
Contributor

Currently caches topics for 1 hour, checks for removing topics every 30seconds, probably should be configurable.

Is there an opportunity for a longer term solution here? May be valid that some data only gets published every 24 hours for example.

@hardillb
Copy link
Contributor Author

Is there an opportunity for a longer term solution here? May be valid that some data only gets published every 24 hours for example.

These are just starting values, but having a longer caching period is likely to not be useful in it's current form as the cache will get cleared every time the forge app restarts (e.g. every time we do a CI deployment....)

There is a comment in the code about using Redis as a better cache, but this is a MUCH bigger solution

@joepavitt
Copy link
Contributor

Why is the URL /api/v1/teams/:teamId/broker/topicList and not /api/v1/teams/:teamId/broker/topics

The latter is more RESTful?

@knolleary
Copy link
Member

Think about longevity of the cache - and dealing with CI reloading the app.

Some options that come to mind:

  1. Introduce redis (more likely valkey). This will also mean we aren't storing state in memory that won't horizontally scale. However it does add the overhead work on adding another component to the architecture. It is inevitable... the question is whether this is the feature that pushes us into action on it.
  2. Store topic list in existing DB. Don't really want to add this load to the DB in realtime, but could be cached in memory and periodically written to DB in batches (including in the shutdown hook). Doesn't help with scaling (as there is state in memory), but will be persistent across restarts.

In terms of the existing caching TTL values, I'd suggest we default for longer intervals - assuming we (eventually) solve the persistence of state. I'd suggest 25hrs ttl and once a day purging - I don't think the level of usage for in-memory caching will be a problem in the short term for it to be quite generous.

@hardillb
Copy link
Contributor Author

Increasing the cache to 25hours is an easy change, but moving to clearing the cache to once a day, will mean moving that code to the house keeping tasks to get round the restarts.

Will look at that shortly

@hardillb
Copy link
Contributor Author

Stashed the topic cache in the database across restarts

@hardillb
Copy link
Contributor Author

hardillb commented Nov 11, 2024

Need to check that this DB cache actually will work on K8s as I think it starts the new instance, before telling the old one to shutdown.

This means the cache will load from the DB before it's populated. Might need to add a delay (and find away to force the app.settings.get to re-read from the database).

@knolleary
Copy link
Member

@hardillb lets discuss before you throw commits at the PR

Should work for K8s restarts
forge/lib/permissions.js Outdated Show resolved Hide resolved
hardillb and others added 2 commits November 13, 2024 19:45
Co-authored-by: Nick O'Leary <nick.oleary@gmail.com>
@joepavitt
Copy link
Contributor

joepavitt commented Nov 19, 2024

Is this good to merge? I've not got too many opinions here as it's more server-side work. So if Nick says go, I vote, we go

@joepavitt joepavitt merged commit ccb0563 into main Nov 19, 2024
12 of 13 checks passed
@joepavitt joepavitt deleted the track-used-topics branch November 19, 2024 11:34
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.

3 participants