-
-
Notifications
You must be signed in to change notification settings - Fork 907
RFC: Support source/severity for messages #1180
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
base: master
Are you sure you want to change the base?
Conversation
…data over the network There are two changes to the protocol: - `QueueMessageLiteral*` were changed and what used to be addresses are now addresses+metadata - Other messages now send `QueueMessage*Metadata` with added metadata. This will later be used to store and transmit message sources, level, etc.
For now, define all messages to use `MessageSourceType::User` and `tracy::MessageSeverity::Info` by default. This does not currently change anything functionally except the protocol. Severities were chosen based on the following paper https://arxiv.org/abs/2109.01192
This now uses a single path for all cases (filter/threads changed, new message). Note this no longer discriminates against `m_messageFilter.IsActive()` to skip the message filter. `ImGui::PassFilter already` early outs, and if performance is a concern we might as well start by caching the result of `VisibleMsgThread` which always does a hashmap lookup.
Instead of decompressing and checking visibility of the thread all the time, cache the previous result.
…BOSE` or `TRACY_VERBOSE_NO_MESSAGE` is defined
No,
I think it's a bit premature and can lead to over-engineering.
I don't have a major preference, but A is a bit more compact and feels less busy to the eyes. What does the "Reset" button do with the filters? Will it "check-all", besides clearing the text filter? |
Currently it checks all + clear text (reset all filters). It used to be linked to the text field and was walled Clear.
No, but I suppose we could? One thing that does bother me a little is that you cant easily toggle off all filters but one, with this suggestion this would be possible by typing the severity name. Not sure if it's the best UX though. |
Nah, I think we're good as is. Should people bring this matter in the future, we can always implement it later.
|
@slomp circling back on this, I'm now a bit stuck on the naming for the new macro because we currently have this: #define TracyMessage( txt, size ) tracy::Profiler::Message( txt, size, TRACY_CALLSTACK, tracy::MessageSourceType::User, tracy::MessageSeverity::Info )
#define TracyMessageL( txt ) tracy::Profiler::Message( txt, TRACY_CALLSTACK, tracy::MessageSourceType::User, tracy::MessageSeverity::Info )
#define TracyMessageC( txt, size, color ) tracy::Profiler::MessageColor( txt, size, color, TRACY_CALLSTACK, tracy::MessageSourceType::User, tracy::MessageSeverity::Info )
#define TracyMessageLC( txt, color ) tracy::Profiler::MessageColor( txt, color, TRACY_CALLSTACK, tracy::MessageSourceType::User, tracy::MessageSeverity::Info )
#define TracyMessageS( txt, size, depth ) tracy::Profiler::Message( txt, size, depth, tracy::MessageSourceType::User, tracy::MessageSeverity::Info )
#define TracyMessageLS( txt, depth ) tracy::Profiler::Message( txt, depth, tracy::MessageSourceType::User, tracy::MessageSeverity::Info )
#define TracyMessageCS( txt, size, color, depth ) tracy::Profiler::MessageColor( txt, size, color, depth, tracy::MessageSourceType::User, tracy::MessageSeverity::Info )
#define TracyMessageLCS( txt, color, depth ) tracy::Profiler::MessageColor( txt, color, depth, tracy::MessageSourceType::User, tracy::MessageSeverity::Info )I can't seem to find a good letter for the versions with severity levels. This leads me to the following questions:
|
The exploding number of macro combinations is actually a problem. |
|
In my opinion, we should introduce just one new "overarching" macro to the public API: and leave further macroing and abstractions entirely up to the user. |
I considered that too to limit the number of potential macros, but I think we need a way to at least differentiate between literal and non-literal to avoid unnecessary copies? or we could use some size hint for litterals such that we could check for it (for example
I'd very much like to avoid it too. If we were to break compat, I would simply expose color+source+severity as a single param "metadata". I'm not sure I would even expose all the depth stack variants (I tend to think that if someone want to customize depth of callstack captures, they can just have their own macros). So unless we break the current API (which we most likely do not want), or limit the number of macros to not expose every variant (but then we expose ourselves to questions such as "why variant XYZ does not exist), I think the best it to go the "on definition that covers all use-cases, users will create their own wrapper" such as suggested above is the best way to go. So to sum up I'm thinking of either: or The only other solution I have in mind would be to use variadics and let the overload resolution of |
This pull request introduces some new metadata to messages: sources and severity. The main objective was to provide a way to send internal information to the user by default, such as errors related to system sampling for example.
Currently sources are User/Tracy, and severity levels are
Trace, Debug, Info, Warning, Error, Fatal, based on https://arxiv.org/abs/2109.01192 .Note that some commits are not finalized / meant to be merged as is, as I'd like some feedback first on the following questions.
TracyMessage*defines?tracy::MessageSourceType::Userandtracy::MessageSeverity::Infothe correct defaults for those?Still needs to be done:
TracyDebugmessages by splitting into multiple macrosAny kind of feedback is welcome.