[core][obsclean/04] move metric registration to constructor #55544
Conversation
This file contains hidden or 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
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors metric registration by moving it from the Record method into the constructors of metric classes. This is a good change for performance and clarity. However, I've identified a potential thread-safety issue where a mutex was removed during this refactoring, which could lead to race conditions. I've also pointed out an opportunity to reduce code duplication in the derived metric classes to improve maintainability. Please see the detailed comments for suggestions.
can-anyscale
changed the title
can-anyscale
force-pushed
the
can-obsclean04
branch
2 times, most recently
from
5210e2a to
12139b6
Compare
can-anyscale
force-pushed
the
can-obsclean03
branch
from
ad514ea to
e9162d1
Compare
can-anyscale
force-pushed
the
can-obsclean04
branch
from
12139b6 to
af3804d
Compare
can-anyscale
force-pushed
the
can-obsclean04
branch
2 times, most recently
from
2705ff8 to
0750a73
Compare
can-anyscale
force-pushed
the
can-obsclean04
branch
from
0750a73 to
0f037e3
Compare
can-anyscale
force-pushed
the
can-obsclean04
branch
from
0f037e3 to
a60c493
Compare
can-anyscale
force-pushed
the
can-obsclean04
branch
from
a60c493 to
b6a8c2c
Compare
ray-gardener
bot
added
core
can-anyscale
force-pushed
the
can-obsclean04
branch
2 times, most recently
from
bd6f10c to
4e6d29a
Compare
Signed-off-by: Cuong Nguyen <[email protected]>
can-anyscale
force-pushed
the
can-obsclean04
branch
from
4e6d29a to
9e868ce
Compare
edoakes
approved these changes
Aug 26, 2025
liulehui
pushed a commit
to liulehui/ray
that referenced
this pull request
Aug 26, 2025
…ect#55544) Currently, the `Metric` class registers the metric name each time it records a new value, due to the [C++ static initialization order fiasco](https://en.cppreference.com/w/cpp/language/siof.html). The base of this PR move all static metrics to runtime initialization, allowing us to safely perform metric registration in the constructor. Note that I'm only doing this for the open-telemetry stack. The opencensus has a complex registration sequence using view+measure and I have hard time make it to work so I don't bother fixing it. Test: - CI Signed-off-by: Cuong Nguyen <[email protected]> Signed-off-by: Lehui Liu <[email protected]>
tohtana
pushed a commit
to tohtana/ray
that referenced
this pull request
Aug 29, 2025
…ect#55544) Currently, the `Metric` class registers the metric name each time it records a new value, due to the [C++ static initialization order fiasco](https://en.cppreference.com/w/cpp/language/siof.html). The base of this PR move all static metrics to runtime initialization, allowing us to safely perform metric registration in the constructor. Note that I'm only doing this for the open-telemetry stack. The opencensus has a complex registration sequence using view+measure and I have hard time make it to work so I don't bother fixing it. Test: - CI Signed-off-by: Cuong Nguyen <[email protected]> Signed-off-by: Masahiro Tanaka <[email protected]>
tohtana
pushed a commit
to tohtana/ray
that referenced
this pull request
Aug 29, 2025
…ect#55544) Currently, the `Metric` class registers the metric name each time it records a new value, due to the [C++ static initialization order fiasco](https://en.cppreference.com/w/cpp/language/siof.html). The base of this PR move all static metrics to runtime initialization, allowing us to safely perform metric registration in the constructor. Note that I'm only doing this for the open-telemetry stack. The opencensus has a complex registration sequence using view+measure and I have hard time make it to work so I don't bother fixing it. Test: - CI Signed-off-by: Cuong Nguyen <[email protected]> Signed-off-by: Masahiro Tanaka <[email protected]>
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.
Currently, the
Metricclass registers the metric name each time it records a new value, due to the C++ static initialization order fiasco. The base of this PR move all static metrics to runtime initialization, allowing us to safely perform metric registration in the constructor.Note that I'm only doing this for the open-telemetry stack. The opencensus has a complex registration sequence using view+measure and I have hard time make it to work so I don't bother fixing it.
Test: