Skip to content
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

common: Deprecate TraceCompassLogUtils and use trace-event-logger lib #207

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PatrickTasse
Copy link
Contributor

@PatrickTasse PatrickTasse commented Jan 23, 2025

What it does

Deprecate TraceCompassLogUtils and make its implementation defer all its
work to LogUtils from the trace-event-logger library.

Replace all use of TraceCompassLogUtils with LogUtils.

Update all targets to include the trace-event-logger jar from Maven.

Add trace-event-logger plug-in to RCP feature and releng-site.

How to test

Unit tests.

Follow-ups

N/A

Review checklist

  • As an author, I have thoroughly tested my changes and carefully followed the instructions in this template

@PatrickTasse PatrickTasse force-pushed the logutils branch 3 times, most recently from d05f6b8 to 1f762a0 Compare January 27, 2025 15:29
return this;
}

/**
* Set a category and ID for the flow scope.
*
* This method is mutually exclusive with
* {@link #setParentScope(FlowScopeLog)}. Calling both will throw an
* {@link #setParentScope(IFlowScopeLog)}. Calling both will throw an
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be change to FlowScopeLog

}

/**
* Set a category for the flow scope. When building the scope, an ID
* will be automatically generated.
*
* This method is mutually exclusive with
* {@link #setParentScope(FlowScopeLog)}. Calling both will throw an
* {@link #setParentScope(IFlowScopeLog)}. Calling both will throw an
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be change to FlowScopeLog

Copy link
Contributor

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a first pass. Please rebase this PR. The jsonify.sh script doesn't work anymore. Maybe copy the python script of the trace-event-logger repository.

With the deprecated logger, it was necessary to set -Dorg.eclipse.tracecompass.logging=true. The TraceCompassLogUtils class had code to disable the logging if that flag is not set. That's to avoid unnecessary logging due to unrelated logging.properties. We need to decide if we need this and where to set it if needed, either trace-event-logger level or in Trace Compass repo only.

The BUILDING.md file needs to be updated for the new usage of the library. Also, the logging.properties that is in the root folder of the repo needs to be updated. Mabye, we should have a couple examples with typical settings for different handlers including the FileHandler.

To make the AsyncFileHandler and SnapshotHandler work, you need to set the bootclasspath. From the RCP you can use the relative path. From the IDE development environment you will have to provide an absolute path.

-Xbootclasspath/a:./plugins/org.eclipse.tracecompass.trace-event-logger_0.2.0.jar
-Djava.util.logging.config.file=goodlogging.properties
-Dorg.eclipse.tracecompass.logging=true

@PatrickTasse
Copy link
Contributor Author

I did a first pass. Please rebase this PR. The jsonify.sh script doesn't work anymore. Maybe copy the python script of the trace-event-logger repository.

With the deprecated logger, it was necessary to set -Dorg.eclipse.tracecompass.logging=true. The TraceCompassLogUtils class had code to disable the logging if that flag is not set. That's to avoid unnecessary logging due to unrelated logging.properties. We need to decide if we need this and where to set it if needed, either trace-event-logger level or in Trace Compass repo only.

The BUILDING.md file needs to be updated for the new usage of the library. Also, the logging.properties that is in the root folder of the repo needs to be updated. Mabye, we should have a couple examples with typical settings for different handlers including the FileHandler.

To make the AsyncFileHandler and SnapshotHandler work, you need to set the bootclasspath. From the RCP you can use the relative path. From the IDE development environment you will have to provide an absolute path.

-Xbootclasspath/a:./plugins/org.eclipse.tracecompass.trace-event-logger_0.2.0.jar
-Djava.util.logging.config.file=goodlogging.properties
-Dorg.eclipse.tracecompass.logging=true

The commit is rebased.

The bash script jsonify.sh is changed to be a wrapper to download and execute jsonify.py from trace-event-logger repository. The downloaded script is added to .gitignore file.

The property -Dorg.eclipse.tracecompass.logging=true is still needed as it activates the Logger instances that are passed to the trace-event-logger library.

Copy link
Contributor

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to create a trace and load it into Trace Compass. Some small comments though.

jsonify.sh Outdated
@@ -1,18 +1,24 @@
#!/usr/bin/env bash
#*******************************************************************************
# Copyright (c) 2020 Ericsson
# Copyright (c) 2025 Ericsson
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be 2020, 2025

@@ -44,7 +44,7 @@ Trace Compass can be traced by doing the following in the launch configuration:
* -Djava.util.logging.config.file=%gitroot%/logging.properties (where %gitroot% is the directory tracecompass is checked out to)
* -Dorg.eclipse.tracecompass.logging=true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add some description here on how to use the other handlers and how to make it work with the -Xbootclasspath setting? We should also have a reference link to the trace-event-logger documentation for more information.

The README.md in this repo should mention about tracing Trace Compass and have link to the chapter in BUILDING.md.

Deprecate TraceCompassLogUtils.

Replace all use of TraceCompassLogUtils with LogUtils from the
trace-event-logger library.

Update all targets to include the trace-event-logger jar from Maven.

Add trace-event-logger plug-in to RCP feature and releng-site.

Signed-off-by: Patrick Tasse <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants