-
Notifications
You must be signed in to change notification settings - Fork 20
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
Implement spans for metrics profiling #1268
base: main
Are you sure you want to change the base?
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1268 +/- ##
=======================================
Coverage 40.61% 40.61%
=======================================
Files 354 354
Lines 26330 26330
=======================================
Hits 10695 10695
Misses 15635 15635 ☔ View full report in Codecov by Sentry. |
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.
Clang-Tidy
found issue(s) with the introduced code (1/2)
std::string name(); | ||
|
||
private: | ||
std::thread::id id; ///< Thread id where span was invoked. |
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.
invalid case style for private member id
std::thread::id id; ///< Thread id where span was invoked. | |
std::thread::id mId; ///< Thread id where span was invoked. |
} | ||
|
||
SpanManager::Span::Span(const std::string& name, std::size_t level, const std::string& file, std::size_t line) | ||
: id(std::this_thread::get_id()) |
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.
invalid case style for private member id
: id(std::this_thread::get_id()) | |
: mId(std::this_thread::get_id()) |
std::string name(); | ||
|
||
private: | ||
std::thread::id id; ///< Thread id where span was invoked. |
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.
invalid case style for private member id
std::thread::id id; ///< Thread id where span was invoked. | |
std::thread::id mId; ///< Thread id where span was invoked. |
std::thread::id id; ///< Thread id where span was invoked. | ||
std::string mName; ///< The name of the scope. | ||
std::string mFile; ///< The file from where it was invoked. | ||
std::size_t mLine; ///< The line from where it was invoked. |
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.
private field mLine
is not used
std::string mFile; ///< The file from where it was invoked. | ||
std::size_t mLine; ///< The line from where it was invoked. | ||
std::chrono::time_point<std::chrono::high_resolution_clock> mStart; ///< Start time when constructed. | ||
int mLevel; |
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.
private field mLevel
is not used
return state; | ||
} | ||
|
||
SpanManager::Span::Span(const std::string& name, std::size_t level, const std::string& file, std::size_t line) |
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.
pass by value and use std::move
|
||
SpanManager::Span::Span(const std::string& name, std::size_t level, const std::string& file, std::size_t line) | ||
: id(std::this_thread::get_id()) | ||
, mLevel(level) |
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.
field mLevel
will be initialized after field mName
, mLevel(level) |
, mFile(file) | ||
, mLine(line) | ||
, mStart(std::chrono::time_point<std::chrono::high_resolution_clock>::clock::now()) | ||
{ |
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.
field mLevel
will be initialized after field mName
{ | |
, mLevel(level) | |
{ |
|
||
SpanManager::Span::Span(const std::string& name, std::size_t level, const std::string& file, std::size_t line) | ||
: id(std::this_thread::get_id()) | ||
, mLevel(level) |
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.
narrowing conversion from std::size_t
(aka unsigned long
) to signed type int
is implementation-defined
|
||
SpanManager::Span::Span(const std::string& name, std::size_t level, const std::string& file, std::size_t line) | ||
: id(std::this_thread::get_id()) | ||
, mLevel(level) |
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.
implicit conversion loses integer precision: std::size_t
(aka unsigned long
) to int
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.
Clang-Tidy
found issue(s) with the introduced code (2/2)
SpanManager::Span::~Span() | ||
{ | ||
auto now = std::chrono::time_point<std::chrono::high_resolution_clock>::clock::now(); | ||
std::chrono::duration<double, std::milli> elapsed = now - mStart; |
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.
Value stored to elapsed
during its initialization is never read
SpanManager::Span::~Span() | ||
{ | ||
auto now = std::chrono::time_point<std::chrono::high_resolution_clock>::clock::now(); | ||
std::chrono::duration<double, std::milli> elapsed = now - mStart; |
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.
unused variable elapsed
Description
See linked issue for proposed implementation, but in short:
tel
with metrics, tracing and logging.Checklist