-
Notifications
You must be signed in to change notification settings - Fork 537
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 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
… them multiple times (#9914) (#9930) * Don't hold labels from store-gateways in two forms * Don't retain labels longer than needed * Don't convert mimirpb.LabelAdaptors to labels.Labels multiple times * Add changelog entry (cherry picked from commit d2367de) Co-authored-by: Charles Korn <[email protected]>
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.
|
Signed-off-by: Nikos Angelopoulos <[email protected]> (cherry picked from commit 0caede4) Co-authored-by: Nikos Angelopoulos <[email protected]>
The result of ingester push benchmark, the impact is less compare to distributor.
|
9b8ae76
to
e0525e1
Compare
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.