Add support for forwarding trace messages to logs #726
Conversation
Initially, support for tracing to DLT was added. Later, support for forwarding trace messages to logs on Android was added. This commit adds support for forwarding trace messages to logs on all platforms by introducing the build flag TRACE_TO_LOGS. If either USE_DLT or TRACE_TO_LOGS is defined when building vsomeip, support for generating trace messages will be included in the library. The Android.bp file has been updated to include TRACE_TO_LOGS instead of USE_DLT, which allows removal of a number of additional ifdef guards around DLT-specific code. Co-authored-by: Daniel Freiermuth <[email protected]>
|
After reviewing this PR with our internal team, we decided that the cost to benefit of this change, the testing and integration of this for the benefit of removing some of the ifdef guards would not pay off. |
|
@duartenfonseca Thank you for taking the time to review this PR. The primary goal of this change is to extend support for outputting trace information to logs, beyond just DLT. While the main branch already allows trace output to logs on Android, this update enables similar functionality across other platforms where it can also be valuable. The key addition in this PR can be seen here: link. This change introduces a TRACE_TO_LOGS directive, which decouples the logging functionality from the existing Android-specific ifdef. The intent is to clarify that tracing can be directed to both DLT (USE_DLT) and logs (TRACE_TO_LOGS) across all platforms, not just Android. I would appreciate it if you could reconsider the decision to close this PR. If there are any adjustments or improvements you’d like to see to better align with the project's goals, I’m more than happy to make those changes to ensure that tracing to logs is supported on platforms beyond Android. |
|
@falk-haleytek i will take another look at this changes. thank you for the input |
| std::string app = runtime::get_property("LogApplication"); | ||
| ALOGI(app.c_str(), ss.str().c_str()); | ||
| #else | ||
| VSOMEIP_INFO << ss.str(); |
There was a problem hiding this comment.
| VSOMEIP_INFO << ss.str(); | |
| VSOMEIP_TRACE << ss.str(); |
|
@falk-haleytek would you agree with reverting the change of TRACE_TO_LOGS, which would be hard to change internally, but keep the added functionality of printing TC logs to the console? the suggestions I added would be for that purpose, but it would also mean that you would revert all the other TRACE_TO_LOGS changes |
|
Thanks for the new suggestions, @duartenfonseca. Could you clarify what makes the TRACE_TO_LOGS change difficult to implement internally? Understanding this would help us find a solution that addresses your concerns. Currently, the USE_DLT define seems to be somewhat misaligned with its intended use. It was introduced by default in the Android build, possibly to enable tracing to logs (?). However, this is effectively negated by other additions, such as this one, which guards against the USE_DLT define if it detects an Android build. This creates a situation where USE_DLT must be defined for Android, even though Android does not support DLT. The introduction of a new define (TRACE_TO_LOGS) with a clear intent is meant to address this technical debt. It decouples the logging functionality from the Android-specific ifdef, making it clear that tracing can be directed to logs across all platforms, not just Android. If there’s a way to retain the added functionality while addressing your internal concerns, I’m open to adjusting the implementation. I believe it’s important to resolve this in a way that benefits all platforms without introducing further confusion. |
what we were trying to avoid was changes that would involve many files on the repo, because testing and integration of those changes on android side is cumbersome. but from our investigation there is no way around it. |
VSOMEIP_INFO already provides the same functionality out of the box.
Co-authored-by: Duarte Fonseca <[email protected]>
|
@duartenfonseca Just to avoid misunderstandings, are you waiting for anymore input or adjustments from me here? |
|
@falk-haleytek we are discussing internally adding this fix to others related to the logger, will reach back to you when that is done |
|
@duartenfonseca got it! thanks for the update. |
|
@duartenfonseca any updates on the internal discussions? |
some other issues have appeared, but i will try to get back to this |
Initially, support for tracing to DLT was added. Later, support for forwarding trace messages to logs on Android was added. This commit adds support for forwarding trace messages to logs on all platforms by introducing the build flag TRACE_TO_LOGS.
If either USE_DLT or TRACE_TO_LOGS is defined when building vsomeip, support for generating trace messages will be included in the library.
The Android.bp file has been updated to include TRACE_TO_LOGS instead of USE_DLT, which allows removal of a number of additional ifdef guards around DLT-specific code.