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.

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

Expose cache ttl for es span writer index+service #2737

Merged
merged 3 commits into from
Jan 21, 2021
Merged

Expose cache ttl for es span writer index+service #2737

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
715640a
cache ttl for es span writer index+service
Tyghe Jan 21, 2021
8a8182c
renamed variables, added tests
Tyghe Jan 21, 2021
b8afaff
switching to time.Duration
Tyghe Jan 21, 2021
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
Next Next commit
cache ttl for es span writer index+service 
Signed-off-by: Tyghe Vallard <vallardt@gmail.com>
Signed-off-by: Tyghe Vallard <tyghe.vallard@target.com>
  • Loading branch information
@Tyghe
Tyghe committed Jan 21, 2021
commit 715640a26f352a7ff6d2ef879981f31d251edcfb
53 changes: 38 additions & 15 deletions plugin/storage/es/spanstore/writer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ import (
)

const (
spanType = "span"
serviceType = "service"
spanType = "span"
serviceType = "service"
serviceCacheDurationDefault = 12 * time.Hour
indexCacheDurationDefault = 48 * time.Hour
)

type spanWriterMetrics struct {
Expand All @@ -53,22 +55,43 @@ type SpanWriter struct {

// SpanWriterParams holds constructor parameters for NewSpanWriter
type SpanWriterParams struct {
Client es.Client
Logger *zap.Logger
MetricsFactory metrics.Factory
IndexPrefix string
IndexDateLayout string
AllTagsAsFields bool
TagKeysAsFields []string
TagDotReplacement string
Archive bool
UseReadWriteAliases bool
Client es.Client
Logger *zap.Logger
MetricsFactory metrics.Factory
IndexPrefix string
IndexDateLayout string
AllTagsAsFields bool
TagKeysAsFields []string
TagDotReplacement string
Archive bool
UseReadWriteAliases bool
ServiceCacheDuration string
IndexCacheDuration string
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ServiceCacheDuration string
IndexCacheDuration string
ServiceCacheDuration time.Duration
IndexCacheDuration time.Duration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You cannot check if a time.Duration is set or not since it would be 0, but you can check if a string is empty which is why I went the path I did there.

Anybody directly using the writer in their own code can use it and if later somebody wants to plumb in the ability for a flag for the apps to be able to set these could do that in a separate PR

I'm not sure how to test this since everything is in a private property and I cannot check if they are set in a test

Copy link
Member

Choose a reason for hiding this comment

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

What's wrong with 0? 0 is not a valid value for these parameters, so it works perfectly well as "unset" value.

Tests run in the private package namespace and can test those values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0 is a valid value and would indicate caching forever so you still would not be able to differentiate from cache forever and not being set. Unless you want to assume that nobody would ever want to cache forever(which could be a valid thing, but I would rather be able to know for sure)

I did figure out how to test and just pushed those

Copy link
Member

@yurishkuro yurishkuro Jan 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

0 is a valid value and would indicate caching forever

Technically, for the LRU cache that is used under the hood, the ttl=0 is a valid param. But in the context of the specific caching scenarios here (index and service names) I do not believe 0 is a valid value, because these caches grow over time. So we can use 0 as "undefined". If people want to cache "forever" they can still do it by providing a very long duration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I can make the change

Copy link
Member

Choose a reason for hiding this comment

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

If you really want to support "undefined" then the other alternative is to use a pointer.

}

// NewSpanWriter creates a new SpanWriter for use
func NewSpanWriter(p SpanWriterParams) *SpanWriter {
// TODO: Configurable TTL
serviceOperationStorage := NewServiceOperationStorage(p.Client, p.Logger, time.Hour*12)
var err error

serviceCacheDuration := serviceCacheDurationDefault
if p.ServiceCacheDuration != "" {
serviceCacheDuration, err = time.ParseDuration(p.ServiceCacheDuration)
if err != nil {
p.Logger.Warn("error parsing service cache duration. Using default", zap.Error(err), zap.Duration("default", serviceCacheDurationDefault))
serviceCacheDuration = serviceCacheDurationDefault
}
}

indexCacheDuration := indexCacheDurationDefault
if p.ServiceCacheDuration != "" {
indexCacheDuration, err = time.ParseDuration(p.IndexCacheDuration)
if err != nil {
p.Logger.Warn("error parsing index cache duration. Using default", zap.Error(err), zap.Duration("default", indexCacheDurationDefault))
indexCacheDuration = indexCacheDurationDefault
}
}

serviceOperationStorage := NewServiceOperationStorage(p.Client, p.Logger, serviceCacheDuration)
return &SpanWriter{
client: p.Client,
logger: p.Logger,
Expand All @@ -79,7 +102,7 @@ func NewSpanWriter(p SpanWriterParams) *SpanWriter {
indexCache: cache.NewLRUWithOptions(
5,
&cache.Options{
TTL: 48 * time.Hour,
TTL: indexCacheDuration,
},
),
spanConverter: dbmodel.NewFromDomain(p.AllTagsAsFields, p.TagKeysAsFields, p.TagDotReplacement),
Expand Down