Remove user from state key metric value #5453
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
qinxx108
force-pushed
the
remove-user-from-state-key-metric-value
branch
from
83dd411 to
0a9d0f4
Compare
yeya24
reviewed
Jul 13, 2023
| @@ -168,19 +168,19 @@ func newAlertmanagerMetrics() *alertmanagerMetrics { | |||
| partialMerges: prometheus.NewDesc( | |||
| "cortex_alertmanager_partial_state_merges_total", | |||
| "Number of times we have received a partial state to merge for a key.", | |||
| []string{"user"}, nil), | |||
| []string{"user", "key"}, nil), | |||
There was a problem hiding this comment.
What will be the value of the key label? I am okay to add this as seems this is what we have in AM so I am not worried about cardinality issue. Just out of curiosity.
There was a problem hiding this comment.
Discussed with @qinxx108 offline, this won't increase cardinality.
qinxx108
force-pushed
the
remove-user-from-state-key-metric-value
branch
from
0a9d0f4 to
bdc940f
Compare
alvinlin123
reviewed
Jul 13, 2023
| s.partialStateMergesFailed.WithLabelValues(key) | ||
| s.stateReplicationTotal.WithLabelValues(key) | ||
| s.stateReplicationFailed.WithLabelValues(key) | ||
| s.partialStateMergesTotal.WithLabelValues(getKeyWithoutUser(key)) |
There was a problem hiding this comment.
It would be better to do keyWithoutUser = getKeyWithoutUser(key) then use the new variable to pass into WithLabelValues
|
|
||
| // getKeyWithoutUser used for trim the userID out of the metric label value. | ||
| func getKeyWithoutUser(key string) string { | ||
| if strings.IndexByte(key, ':') < 0 { |
There was a problem hiding this comment.
Why call IndexByte twice? We can save the result in a variable right?
qinxx108
force-pushed
the
remove-user-from-state-key-metric-value
branch
from
bdc940f to
278f184
Compare
* Implementing Bucket index sync status Signed-off-by: Alan Protasio <alanprot@gmail.com> * fixing bug when returning from cache Signed-off-by: Alan Protasio <alanprot@gmail.com> * Addressing some comments Signed-off-by: Alan Protasio <alanprot@gmail.com> * Changelog Signed-off-by: Alan Protasio <alanprot@gmail.com> --------- Signed-off-by: Alan Protasio <alanprot@gmail.com> Signed-off-by: Yijie Qin <qinyijie@amazon.com>
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
qinxx108
force-pushed
the
remove-user-from-state-key-metric-value
branch
from
25e9b6f to
d6f1b06
Compare
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
alvinlin123
approved these changes
Jul 13, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does:
The metric helper will remove the userID label and aggregate the metric, however we have a key label value in the state replicator which cause a memory leak on alert manager since the key label value include the userId which is not getting removed. This PR will remove the userId from the key
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]