Skip to content

Implement TTL for cached datasource instances #1230

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.

Sign up for GitHub

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

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

Implement TTL for cached datasource instances #1230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4110d1d
WIP: Implementing TTL for cached datasource instances
iwysiu Feb 12, 2025
2ccc384
increase expiration timeout
iwysiu Apr 15, 2025
a3ed029
fix race condition in test
iwysiu Apr 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 32 additions & 20 deletions backend/instancemgmt/instance_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@ package instancemgmt

import (
"context"
"fmt"
"reflect"
"sync"
"time"

"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promauto"

"github.com/grafana/grafana-plugin-sdk-go/backend"
gocache "github.com/patrickmn/go-cache"
)

var (
Expand All @@ -18,7 +19,10 @@ var (
Name: "active_instances",
Help: "The number of active plugin instances",
})
disposeTTL = 5 * time.Second
defaultDisposeTTL = 5 * time.Second

defaultInstanceTTL = 24 * time.Hour
defaultInstanceCleanup = 48 * time.Hour
)

// Instance is a marker interface for an instance.
Expand Down Expand Up @@ -73,21 +77,35 @@ type InstanceProvider interface {

// New create a new instance manager.
func New(provider InstanceProvider) InstanceManager {
return NewWithOptions(provider, defaultInstanceTTL, defaultInstanceCleanup, defaultDisposeTTL)
}

func NewWithOptions(provider InstanceProvider, instanceTTL, instanceCleanup, disposeTTL time.Duration) InstanceManager {
if provider == nil {
panic("provider cannot be nil")
}
cache := gocache.New(instanceTTL, instanceCleanup)
cache.OnEvicted(func(_ string, i interface{}) {
ci := i.(CachedInstance)
if disposer, valid := ci.instance.(InstanceDisposer); valid {
time.AfterFunc(disposeTTL, func() {
disposer.Dispose()
})
}
activeInstances.Dec()
})

return &instanceManager{
provider: provider,
cache: sync.Map{},
cache: cache,
locker: newLocker(),
}
}

type instanceManager struct {
locker *locker
provider InstanceProvider
cache sync.Map
cache *gocache.Cache
}

func (im *instanceManager) Get(ctx context.Context, pluginContext backend.PluginContext) (Instance, error) {
Expand All @@ -96,9 +114,10 @@ func (im *instanceManager) Get(ctx context.Context, pluginContext backend.Plugin
return nil, err
}
// Double-checked locking for update/create criteria
im.locker.RLock(cacheKey)
item, ok := im.cache.Load(cacheKey)
im.locker.RUnlock(cacheKey)
strKey := fmt.Sprintf("%v", cacheKey)
im.locker.RLock(strKey)
item, ok := im.cache.Get(strKey)
im.locker.RUnlock(strKey)
Comment on lines +117 to +120
Copy link
Contributor

Choose a reason for hiding this comment

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

the shared read locking here looks unnecessary. even though your PR didn't introduce it, I'd be in favour of removing it.


if ok {
ci := item.(CachedInstance)
Expand All @@ -109,35 +128,28 @@ func (im *instanceManager) Get(ctx context.Context, pluginContext backend.Plugin
}
}

im.locker.Lock(cacheKey)
defer im.locker.Unlock(cacheKey)
im.locker.Lock(strKey)
defer im.locker.Unlock(strKey)

if item, ok := im.cache.Load(cacheKey); ok {
if item, ok := im.cache.Get(strKey); ok {
ci := item.(CachedInstance)
needsUpdate := im.provider.NeedsUpdate(ctx, pluginContext, ci)

if !needsUpdate {
return ci.instance, nil
}

if disposer, valid := ci.instance.(InstanceDisposer); valid {
time.AfterFunc(disposeTTL, func() {
disposer.Dispose()
activeInstances.Dec()
})
} else {
activeInstances.Dec()
}
im.cache.Delete(strKey)
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be a little faster to avoid this delete, make a function for the disposer code, and call that function from here and in the OnEvicted handler.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the added complexity probably isn't worth it here - Delete here does acquire a lock, but we'll only hit this code path when the datasource config or Grafana config are updated, so quite rarely in the scheme of things.

}

instance, err := im.provider.NewInstance(ctx, pluginContext)
if err != nil {
return nil, err
}
im.cache.Store(cacheKey, CachedInstance{
im.cache.Set(strKey, CachedInstance{
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like there's a cache.SetDefault() method for the 'set this and use the default expiration' case.

PluginContext: pluginContext,
instance: instance,
})
}, gocache.DefaultExpiration)
activeInstances.Inc()

return instance, nil
Expand Down
53 changes: 40 additions & 13 deletions backend/instancemgmt/instance_manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ func TestInstanceManager(t *testing.T) {
}

tip := &testInstanceProvider{}
im := New(tip)
im := NewWithOptions(tip, defaultInstanceTTL, defaultInstanceCleanup, time.Millisecond)

t.Run("When getting instance should create a new instance", func(t *testing.T) {
instance, err := im.Get(ctx, pCtx)
Expand All @@ -43,11 +43,6 @@ func TestInstanceManager(t *testing.T) {
Updated: time.Now(),
},
}
origDisposeTTL := disposeTTL
disposeTTL = time.Millisecond
t.Cleanup(func() {
disposeTTL = origDisposeTTL
})
newInstance, err := im.Get(ctx, pCtxUpdated)

t.Run("New instance should be created", func(t *testing.T) {
Expand All @@ -70,6 +65,44 @@ func TestInstanceManager(t *testing.T) {
})
}

func TestInstanceManagerExpiration(t *testing.T) {
ctx := context.Background()
pCtx := backend.PluginContext{
OrgID: 1,
AppInstanceSettings: &backend.AppInstanceSettings{
Updated: time.Now(),
},
}

tip := &testInstanceProvider{}
im := NewWithOptions(tip, time.Millisecond, 2*time.Millisecond, defaultDisposeTTL)
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm reading this correctly, this test will take 5 seconds - should we use a smaller dispose TTL here?


instance, err := im.Get(ctx, pCtx)
require.NoError(t, err)
require.NotNil(t, instance)
require.Equal(t, pCtx.OrgID, instance.(*testInstance).orgID)
require.Equal(t, pCtx.AppInstanceSettings.Updated, instance.(*testInstance).updated)

t.Run("After expiration", func(t *testing.T) {
instance.(*testInstance).wg.Wait()
require.True(t, instance.(*testInstance).disposed.Load())
require.Equal(t, int64(1), instance.(*testInstance).disposedTimes.Load())

newInstance, err := im.Get(ctx, pCtx)

t.Run("New instance should be created", func(t *testing.T) {
require.NoError(t, err)
require.NotNil(t, newInstance)
require.Equal(t, pCtx.OrgID, newInstance.(*testInstance).orgID)
require.Equal(t, pCtx.AppInstanceSettings.Updated, newInstance.(*testInstance).updated)
})

t.Run("New instance should not be the same as old instance", func(t *testing.T) {
require.NotSame(t, instance, newInstance)
})
})
}

func TestInstanceManagerConcurrency(t *testing.T) {
t.Run("Check possible race condition issues when initially creating instance", func(t *testing.T) {
ctx := context.Background()
Expand Down Expand Up @@ -110,12 +143,6 @@ func TestInstanceManagerConcurrency(t *testing.T) {
})

t.Run("Check possible race condition issues when re-creating instance on settings update", func(t *testing.T) {
origDisposeTTL := disposeTTL
disposeTTL = time.Millisecond
t.Cleanup(func() {
disposeTTL = origDisposeTTL
})

ctx := context.Background()
initialPCtx := backend.PluginContext{
OrgID: 1,
Expand All @@ -124,7 +151,7 @@ func TestInstanceManagerConcurrency(t *testing.T) {
},
}
tip := &testInstanceProvider{}
im := New(tip)
im := NewWithOptions(tip, defaultInstanceTTL, defaultInstanceCleanup, time.Millisecond)
// Creating initial instance with old contexts
instanceToDispose, _ := im.Get(ctx, initialPCtx)

Expand Down
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ require (
github.com/oasdiff/yaml3 v0.0.0-20250309153720-d2182401db90 // indirect
github.com/oklog/run v1.0.0 // indirect
github.com/oklog/ulid v1.3.1 // indirect
github.com/patrickmn/go-cache v2.1.0+incompatible
github.com/perimeterx/marshmallow v1.1.5 // indirect
github.com/pierrec/lz4/v4 v4.1.22 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
Expand Down
2 changes: 2 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,8 @@ github.com/oklog/ulid v1.3.1 h1:EGfNDEx6MqHz8B3uNV6QAib1UR2Lm97sHi3ocA6ESJ4=
github.com/oklog/ulid v1.3.1/go.mod h1:CirwcVhetQ6Lv90oh/F+FBtV6XMibvdAFo93nm5qn4U=
github.com/olekukonko/tablewriter v0.0.5 h1:P2Ga83D34wi1o9J6Wh1mRuqd4mF/x/lgBS7N7AbDhec=
github.com/olekukonko/tablewriter v0.0.5/go.mod h1:hPp6KlRPjbx+hW8ykQs1w3UBbZlj6HuIJcUGPhkA7kY=
github.com/patrickmn/go-cache v2.1.0+incompatible h1:HRMgzkcYKYpi3C8ajMPV8OFXaaRUnok+kx1WdO15EQc=
github.com/patrickmn/go-cache v2.1.0+incompatible/go.mod h1:3Qf8kWWT7OJRJbdiICTKqZju1ZixQ/KpMGzzAfe6+WQ=
github.com/perimeterx/marshmallow v1.1.5 h1:a2LALqQ1BlHM8PZblsDdidgv1mWi1DgC2UmX50IvK2s=
github.com/perimeterx/marshmallow v1.1.5/go.mod h1:dsXbUu8CRzfYP5a87xpp0xq9S3u0Vchtcl8we9tYaXw=
github.com/pierrec/lz4/v4 v4.1.22 h1:cKFw6uJDK+/gfw5BcDL0JL5aBsAFdsIT18eRtLj7VIU=
Expand Down
Loading