-
Notifications
You must be signed in to change notification settings - Fork 535
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
POC: Cost Attribution Proposal 1.2 #9733
base: main
Are you sure you want to change the base?
Conversation
1d711e5
to
e7bf88c
Compare
pkg/costattribution/tracker.go
Outdated
|
||
// we need the time stamp, since active series could have entered active stripe long time ago, and already evicted | ||
// from the observed map but still in the active Stripe | ||
func (t *Tracker) DecrementActiveSeries(lbs labels.Labels, value int64, ts time.Time) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here we might still want ts, when we decrement a active series counter that are "active" we may want to keep the timestamp updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we want to update the timestamp of something that didn't have activity?
fa91187
to
00d8c00
Compare
00d8c00
to
4e75934
Compare
5b093ab
to
0456d94
Compare
I reviewed mostly the existing code changes, skipping the costattribution package implementation for now. Please ping me back when this is addressed. Meanwhile, while I didn't look into |
Signed-off-by: Ying WANG <[email protected]>
Signed-off-by: Ying WANG <[email protected]>
Signed-off-by: Ying WANG <[email protected]>
pkg/costattribution/tracker.go
Outdated
"go.uber.org/atomic" | ||
) | ||
|
||
type Tracker interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need an interface here, it just makes things more complex and slow. Now we can't compare two trackers by just comparing the pointers, and we expose too many internal details on exported method just to make that comparison.
There are only two implementations of Tracker: the one that does things and the one that is disabled, we can make the disabled one be the nil pointer of TrackerImpl
(please rename that back to Tracker).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed the interface, and it checks for nil pointer in the receiver instead. commit 7f0b372
func (c *ActiveSeries) ConfigDiffers(ctCfg asmodel.CustomTrackersConfig, caCfg costattribution.Tracker) bool { | ||
if ctCfg.String() != c.CurrentConfig().String() { | ||
return true | ||
} | ||
return !areTrackersEqual(caCfg, c.CurrentCostAttributionTracker()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have ConfigDiffers
now, we don't need CurrentConfig
or CurrentCostAttributionTracker
methods, and we don't need to take mutex twice for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in this commit 698a5c6#diff-74600e3203a9faaa94f7231d161362da3e3223aa024eb324b903e5fc27bb9f3dR116
Based on the benchmark results, the successfully pushed case shows a significant increase in allocations, which is expected due to tracking counters per cost label value, previously there is only one counter for received sample tracking. In discarded cases, differences are minimal since counters per reason were already tracked, with a notable percentage of memory consumption. The additional allocations primarily stem from tracking cost attribution-specific data.
|
The result of ingester push benchmark, the impact is less compare to distributor.
|
What this PR does
Which issue(s) this PR fixes or relates to
Fixes #
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]
.about-versioning.md
updated with experimental features.