fix: locking in metrics #14144
Merged
fix: locking in metrics #14144
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
Joibel
force-pushed
the
metrics-locking
branch
from
733d09f to
1dd5274
Compare
Signed-off-by: Alan Clucas <[email protected]>
Joibel
force-pushed
the
metrics-locking
branch
from
1dd5274 to
c6c12a7
Compare
terrytangyuan
approved these changes
Feb 4, 2025
|
Great fix. |
Joibel
added a commit
that referenced
this pull request
Feb 6, 2025
Signed-off-by: Alan Clucas <[email protected]> (cherry picked from commit e79d1c5)
Joibel
added a commit
that referenced
this pull request
Feb 6, 2025
Signed-off-by: Alan Clucas <[email protected]> (cherry picked from commit e79d1c5)
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.
Fixes #14076
Also fixes my own bad test case (deprecated schedule test)
Motivation
The golang map in the userdata for custom metrics was accessed under some circumstances from multiple go routines without an appropriate mutex - as can be seen from the stack trace and panic in #14076
Modifications
Make the three maps which can be accessed have their own semaphore, instead of attempting to use the single top level one.
Make accessor functions for accessing
instrumentsfrom util/telemetry/metrics so that locks there can be easily reasoned about. (Renamed fromAllInstruments).Make a separate lock for
realtimeWorkflowsfrom util/metrics/metrics and manually implement that.Make a lock for custom_metric UserData, because that is needed in limited scenarios. Don't lock when this is accessed from test framework for cleanness in the framework. I don't believe the lock would be useful as the tests are not free running.
Fixed the deprecations metric test which only passed due to a different CronWorkflow using
schedulenotschedules.Verification
Ensure existing tests pass - all changes are just for code compatibility, not changes to the tests.