Cache collision fix #106

varas · 2018-06-06T08:11:58Z

Description of the changes

Fixes cache collision on the store for rate&deltas when a same metric name is used.

Attributes are supported to namespace the metric name on the NewSet constructor.

Fix test at TestNewSet_Attr_SolvesCacheCollision https://github.com/newrelic/infra-integrations-sdk/pull/106/files#diff-d41479d78f43948797a5970be862f5e0R196

PR Review Checklist

Author

add a risk label after carefully considering the "blast radius" of your changes
describe the intent of your changes in the description. don't just rewrite your code in prose
assign at least one reviewer

Reviewer

review code for readability
verify that high risk behavior changes are well tested
check license for any new external dependency
ask questions about anything that isn't clear and obvious
approve the PR when you consider it's good to merge

mariomac · 2018-06-07T09:39:34Z

data/metric/metrics.go

+type Attributes []Attribute
+
+// Len ...
+func (a Attributes) Len() int { return len(a) }


What are Len, Swap and Less used for?

it's on the comment above: https://github.com/newrelic/infra-integrations-sdk/pull/106/files#diff-83416e4d39b0688a4bec5f4a2c076486R196

Required for Go < v.18, as these do not include sort.Slice

you can see that change isolated in the commit 7da0608

Hmm ok. I forgot the implicit implementation of interfaces in Go.

This reverts commit 3c254fe.

smoya · 2018-06-08T09:46:29Z

data/metric/metrics.go

+	// NSSeparator is the metric namespace separator
+	NSSeparator = "::"
+	// NSAttributeSeparator is the metric attribute key-value separator applied to generate the metric ns.
+	NSAttributeSeparator = "=="


Minor: Shall we use | just to keep consistency with our entity GUID spec?

This is just a disk-storage implementation detail. Nothing related to be managed by user, neither to be pushed to agent. So entity naming here would be indeed misleading.

What I'd do is privatize these consts

smoya · 2018-06-08T09:48:19Z

data/metric/metrics.go

+// Errors
+var (
+	ErrNonNumeric        = errors.New("non-numeric value for rate/delta")
+	ErrNoStoreToCalcDiff = errors.New("can't use deltas nor rates without persistent store")


Avoid using contractions on error messages. You can use cannot instead.

what's the gain on switching ErrNoStoreToCalcDiffto ErrCannotCalcDiffWithoutStore?

I'm talking about the error message.

ops, sure 👍

smoya · 2018-06-08T09:52:59Z

data/metric/metrics.go

@@ -14,6 +15,12 @@ import (
 // each one.
 type SourceType int

+// Attribute represents an attribute metric in key-value pair format.
+type Attribute struct {


Why not declaring Attribute as map[string]string ? Or why not using directly a map as users will not need to use that custom type?

Oh I see. You want to keep the previous interface with no attributes specified.

If we don't enforce users to set those values, they will have eventually the same issue we are trying to fix here.

The point on types (besides safety) is expressing intention. So you understand what that data is and what is aimed to do.

The point on enforcing that was discussed with @areina.

We agreed that there is no need for that as most of the users won't need the feature, so enforcing them to solve a problem they don't have is adding undesired complexity.

IMHO we should enforce it since the user is not aware of the real problem when using RATE and DELTA: they may be facing the issue without even being aware of it.

The big issue here, and what I consider we must fix asap is How to friendly let the user know that the metric sets should be unique.

Added error as a warning (does not breaks the logic) when SetMetric on rate/gauge in a metric-set without attributes

https://github.com/newrelic/infra-integrations-sdk/pull/106/files#diff-83416e4d39b0688a4bec5f4a2c076486R98

Finally stopping the execution

smoya · 2018-06-08T10:07:59Z

data/metric/metrics.go

@@ -82,44 +116,98 @@ func (ms *Set) SetMetric(name string, value interface{}, sourceType SourceType)
 	return nil
 }

-func castToNumeric(value interface{}) (float64, error) {
+func castToFloat(value interface{}) (float64, error) {


… attributes

varas · 2018-06-08T14:54:26Z

data/metric/metrics.go

-		storer:  storer,
+// NewSet creates new metrics set, optionally related to a list of attributes. These attributes makes the metric-set unique.
+// If related attributes are used, then new attributes are added.
+// TODO remove obsolete returned error


This is a BC from previous beta, but really nice to have as it removes a BC from SDKv2 =)

smoya

LGTM 👍 Good job @varas !

smoya

💯

smoya · 2018-06-08T15:36:16Z

This PR fixes #48

varas added 4 commits June 4, 2018 16:01

Rf readability

65365fa

Add FileStore test on metrics

927fa2b

Test failing on cache-collision

6001641

NewSet with variadic KV constuctor

12f251a

varas force-pushed the cache-collision branch from 1c73afb to edd77d6 Compare June 6, 2018 09:00

RetatedTo attributes for NewSet

899a47a

varas force-pushed the cache-collision branch from edd77d6 to 899a47a Compare June 6, 2018 09:01

varas added 3 commits June 6, 2018 12:07

Remove unused Timestamps var on inMemoryStore

3c254fe

Typos

aa6691f

Test inMemoryStore.flushCache

3607cdb

varas force-pushed the cache-collision branch 3 times, most recently from 88849fe to fc3cada Compare June 6, 2018 12:16

Namespace func

d79e716

varas force-pushed the cache-collision branch from fc3cada to d79e716 Compare June 6, 2018 12:17

varas changed the title ~~[WIP] Cache collision fix~~ Cache collision fix Jun 6, 2018

varas added bug Categorizes issue or PR as related to a bug. risk:medium labels Jun 6, 2018

varas requested review from mariomac, areina, smoya, MagdaPaj and xino12 and removed request for mariomac June 6, 2018 12:18

varas added 3 commits June 6, 2018 14:24

Rename Attribute constructor

2876648

Support for Go 1.7 & 1.8

7da0608

Update Entity.NewMetricSet

a54e99b

mariomac reviewed Jun 7, 2018

View reviewed changes

mariomac previously approved these changes Jun 7, 2018

View reviewed changes

varas mentioned this pull request Jun 7, 2018

Cache collisions for metric.RATE metrics #48

Closed

3 tasks

Revert "Remove unused Timestamps var on inMemoryStore"

d781a1b

This reverts commit 3c254fe.

varas dismissed mariomac’s stale review via d781a1b June 7, 2018 14:16

Remove Store empty timestamps

f65d40b

smoya reviewed Jun 8, 2018

View reviewed changes

Code review

717ee10

smoya reviewed Jun 8, 2018

View reviewed changes

Doc

d339717

varas force-pushed the cache-collision branch from 3897a1d to 8416145 Compare June 8, 2018 14:12

Return errors when overriding set attributes or adding deltas without…

e9590ca

… attributes

varas force-pushed the cache-collision branch from 8416145 to e9590ca Compare June 8, 2018 14:22

xino12 previously approved these changes Jun 8, 2018

View reviewed changes

varas commented Jun 8, 2018

View reviewed changes

Typo

f0528b8

varas dismissed xino12’s stale review via f0528b8 June 8, 2018 15:14

varas force-pushed the cache-collision branch from d6e5648 to 51065f3 Compare June 8, 2018 15:22

smoya previously approved these changes Jun 8, 2018

View reviewed changes

varas dismissed smoya’s stale review via 7d64e36 June 8, 2018 15:29

Crash on rate/delta with no set attributes

54c2c73

varas force-pushed the cache-collision branch from 7d64e36 to 54c2c73 Compare June 8, 2018 15:30

Merge branch 'master' into cache-collision

b8779df

smoya approved these changes Jun 8, 2018

View reviewed changes

varas merged commit 1ce1d53 into master Jun 8, 2018

varas deleted the cache-collision branch June 8, 2018 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache collision fix #106

Cache collision fix #106

varas commented Jun 6, 2018 •

edited

Loading

mariomac Jun 7, 2018

varas Jun 7, 2018

mariomac Jun 7, 2018

smoya Jun 8, 2018 •

edited

Loading

varas Jun 8, 2018

smoya Jun 8, 2018

varas Jun 8, 2018

smoya Jun 8, 2018

varas Jun 8, 2018

smoya Jun 8, 2018

varas Jun 8, 2018

smoya Jun 8, 2018

smoya Jun 8, 2018

smoya Jun 8, 2018

varas Jun 8, 2018

varas Jun 8, 2018

smoya Jun 8, 2018

smoya Jun 8, 2018

varas Jun 8, 2018

varas Jun 8, 2018

varas Jun 8, 2018

smoya Jun 8, 2018

varas Jun 8, 2018

smoya left a comment

smoya left a comment

smoya commented Jun 8, 2018

Cache collision fix #106

Cache collision fix #106

Conversation

varas commented Jun 6, 2018 • edited Loading

Description of the changes

PR Review Checklist

Author

Reviewer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoya Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoya left a comment

Choose a reason for hiding this comment

smoya left a comment

Choose a reason for hiding this comment

smoya commented Jun 8, 2018

varas commented Jun 6, 2018 •

edited

Loading

smoya Jun 8, 2018 •

edited

Loading