Re: [PR] A way to disable the optimization for filling the stack traces [logging-log4j2]
ppkarwasz commented on PR #3638: URL: https://github.com/apache/logging-log4j2/pull/3638#issuecomment-2848474770 > I see 2 implementations of `StackLocator`, with the newer one relying on `StackWalker`. It is [available on Android](https://developer.android.com/reference/java/lang/StackWalker), though on the most recent versions only (Android 14+). This way, the `StackLocator` strategy can be used on JVM and newer devices while falling back to the slower `AndroidStackLocator` on older devices. > > Not sure how this solution aligns with your current packaging strategy (because you mentioned multi-release JAR). Take it as a wild idea. One of the two versions of `StackLocator` is in `META-INF/versions/9`, so Android can not use it. Maybe there is an option to use that class when compiling, but I never found it. > If I was to refactor it, I would employ the Strategy pattern in `StackLocatorUtil` rather than adding more code to `StackLocator`. This way, while initializing `StackLocatorUtil`, I would inject the existing `StackLocator` for JVM platforms and `AndroidStackLocator` for Android. Yes, probably this is what we should have used from the start. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Added reference to `android-log4j2` repo to the F.A.Q. [logging-log4j2]
neboskreb closed pull request #3643: Added reference to `android-log4j2` repo to the F.A.Q. URL: https://github.com/apache/logging-log4j2/pull/3643 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add option to provide URIs to monitor in addition to the config file [logging-log4j2]
vy commented on code in PR #3501:
URL: https://github.com/apache/logging-log4j2/pull/3501#discussion_r2072434647
##
log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java:
##
@@ -323,10 +325,19 @@ public void start() {
LOGGER.info("Starting configuration {}...", this);
this.setStarting();
if (watchManager.getIntervalSeconds() >= 0) {
-LOGGER.info(
-"Start watching for changes to {} every {} seconds",
-getConfigurationSource(),
-watchManager.getIntervalSeconds());
+if (uris != null && uris.size() > 0) {
+LOGGER.info(
+"Start watching for changes to {} and {} every {}
seconds",
+getConfigurationSource(),
+uris,
+watchManager.getIntervalSeconds());
+watchMonitorUris();
+} else {
+LOGGER.info(
+"Start watching for changes to {} every {} seconds",
+getConfigurationSource(),
+watchManager.getIntervalSeconds());
+}
Review Comment:
... and, this is the wrong place and time to do admit monitor URIs to the
`WatchManager`. Would you move this admission operation (i.e., the logic in
`watchMonitorUris`) to `initializeWatchers`, please?
##
log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java:
##
@@ -767,6 +785,18 @@ protected void doConfigure() {
setParents();
}
+private List convertToJavaNetUris(final List uris) {
+final List javaNetUris = new ArrayList<>();
+for (Uri uri : uris) {
+try {
+javaNetUris.add(new URI(uri.getUri()));
+} catch (URISyntaxException e) {
+LOGGER.error("Error parsing monitor URI: " + uri, e);
Review Comment:
See [this recent discussion on how to handle configuration
failures](https://lists.apache.org/thread/h2oydyk6xld47ljttqvflbt4530o73vw). In
short, configuration failures should result in the entire configuration to
fail, instead of applying in partially. Existing code that doesn't adhere to
this needs to be fixed, but that is another issue.
##
log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java:
##
@@ -323,10 +325,19 @@ public void start() {
LOGGER.info("Starting configuration {}...", this);
this.setStarting();
if (watchManager.getIntervalSeconds() >= 0) {
-LOGGER.info(
-"Start watching for changes to {} every {} seconds",
-getConfigurationSource(),
-watchManager.getIntervalSeconds());
+if (uris != null && uris.size() > 0) {
+LOGGER.info(
+"Start watching for changes to {} and {} every {}
seconds",
+getConfigurationSource(),
+uris,
+watchManager.getIntervalSeconds());
+watchMonitorUris();
+} else {
+LOGGER.info(
+"Start watching for changes to {} every {} seconds",
+getConfigurationSource(),
+watchManager.getIntervalSeconds());
+}
Review Comment:
Can't we simplify this as follows? (Can `uris` be null at all?)
```suggestion
LOGGER.info(
"Start watching for changes to {} and {} every {}
seconds",
getConfigurationSource(),
uris,
watchManager.getIntervalSeconds());
watchMonitorUris();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [I] Allow disabling the optimization for filling the stack traces [logging-log4j2]
vy commented on issue #3639: URL: https://github.com/apache/logging-log4j2/issues/3639#issuecomment-2848726871 @ppkarwasz, @neboskreb, I have the impression that #3638 addressing this issue is merged. Can we close this issue? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adjust GcpLayout JSON to latest format [logging-log4j2]
vy commented on code in PR #3586: URL: https://github.com/apache/logging-log4j2/pull/3586#discussion_r2072437500 ## src/changelog/.2.x.x/3586_improve_GcpLayout.xml: ## @@ -0,0 +1,10 @@ + +http://www.w3.org/2001/XMLSchema-instance"; + xmlns="https://logging.apache.org/xml/ns"; + xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"; + type="changed"> + https://github.com/apache/logging-log4j2/pull/3586"/> + +Adjust GcpLayout JSON template to support automatic timestamp recognition by the Google Cloud Logging. This also changes `exception`, `thread`, `logger` fields a bit, and removes `insertId` field. Review Comment: ```suggestion Update `GcpLayout.json` JSON Template Layout event template to support automatic timestamp recognition by the Google Cloud Logging. This also changes `exception`, `thread`, `logger` fields, and removes `insertId` field. ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [I] Allow disabling the optimization for filling the stack traces [logging-log4j2]
neboskreb commented on issue #3639: URL: https://github.com/apache/logging-log4j2/issues/3639#issuecomment-2848743878 Though the risk that `SecurityManager` is removed from the runtime remains, the immediate problem on Android is fixed. So I believe yes, #3638 can be happily closed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Improve implementations of `LogEvent.toImmutable()` and `ReusableMessage.memento()` [logging-log4j2]
vy commented on code in PR #3171:
URL: https://github.com/apache/logging-log4j2/pull/3171#discussion_r2072427609
##
log4j-core-test/src/test/java/org/apache/logging/log4j/core/async/RingBufferLogEventTest.java:
##
@@ -222,25 +223,31 @@ void testSerializationDeserialization() {
new DummyNanoClock(1));
((StringMap) evt.getContextData()).putValue("key", "value");
-final RingBufferLogEvent other =
SerialUtil.deserialize(SerialUtil.serialize(evt));
-assertThat(other.getLoggerName()).isEqualTo(loggerName);
-assertThat(other.getMarker()).isEqualTo(marker);
-assertThat(other.getLoggerFqcn()).isEqualTo(fqcn);
-assertThat(other.getLevel()).isEqualTo(level);
-assertThat(other.getMessage()).isEqualTo(data);
-assertThat(other.getThrown()).isNull();
-assertThat(other.getThrownProxy()).isEqualTo(new ThrowableProxy(t));
-assertThat(other.getContextData()).isEqualTo(evt.getContextData());
-assertThat(other.getContextStack()).isEqualTo(contextStack);
-assertThat(other.getThreadName()).isEqualTo(threadName);
-assertThat(other.getSource()).isEqualTo(location);
-assertThat(other.getTimeMillis()).isEqualTo(12345);
-assertThat(other.getInstant().getNanoOfMillisecond()).isEqualTo(678);
+final LogEvent other =
SerialUtil.deserialize(SerialUtil.serialize(evt));
+assertThat(other.getLoggerName()).as("Logger
name").isEqualTo(loggerName);
Review Comment:
Nit: I wouldn't add to the complexity with extra `as()` calls, since the
assertion failure line number precisely points to the associated field.
##
log4j-core/src/main/java/org/apache/logging/log4j/core/async/RingBufferLogEventTranslator.java:
##
@@ -124,6 +129,11 @@ public void setBasicValues(
this.nanoClock = aNanoClock;
}
+/**
+ * @deprecated since 2.25.0. {@link RingBufferLogEventTranslator}
instances should only be used on the thread that
+ * created it.
+ */
+@Deprecated
Review Comment:
Since we removed `AsyncLogger::initTranslatorThreadValues`, usages of this
method will result in undefined behavior. Shouldn't we throw an `UnsupportedOE`
in this method?
##
log4j-api/src/main/java/org/apache/logging/log4j/message/ReusableObjectMessage.java:
##
@@ -128,7 +128,9 @@ public void forEachParameter(final ParameterConsumer
action, final S stat
@Override
public Message memento() {
-return new ObjectMessage(obj);
+Message message = new ObjectMessage(obj);
+message.getFormattedMessage();
Review Comment:
I presume you make this call to trigger the stack trace capture. Would you
mind documenting this, please? (Also in other `message/*Message.java` changes.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Lock Antora dependencies [logging-parent]
vy commented on code in PR #367: URL: https://github.com/apache/logging-parent/pull/367#discussion_r2072430211 ## .github/workflows/deploy-site-reusable.yaml: ## @@ -115,6 +105,12 @@ jobs: git config user.name "ASF Logging Services RM" git config user.email priv...@logging.apache.org + # Checking out a new branch will delete the `node_modules` folder + - name: Save Node.js modules +shell: bash +run: | + zip -qX "$RUNNER_TEMP"/node_modules.zip node_modules Review Comment: Shall we add `-0` to speed things up a bit? ## pom.xml: ## @@ -1576,6 +1575,10 @@ npm pre-site + + install github:apache/logging-parent#feature/366-npm-shrinkwrap + Review Comment: I presume this will be fixed before merging this PR? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adapt `merge-dependabot` to RTC [logging-log4j2]
vy commented on code in PR #3603: URL: https://github.com/apache/logging-log4j2/pull/3603#discussion_r2072430665 ## .github/workflows/add-dependabot-changelog.yaml: ## Review Comment: I prefer this to be made a part of `logging-parent` first. If we you can make that happen, I can help with making the next `logging-parent` release. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Disable (temporarily) Dependabot updates [logging-log4j2]
vy commented on PR #3624: URL: https://github.com/apache/logging-log4j2/pull/3624#issuecomment-2848716749 @ppkarwasz, what is long-term solution to this problem? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adjust GcpLayout JSON to latest format [logging-log4j2]
vy commented on PR #3586: URL: https://github.com/apache/logging-log4j2/pull/3586#issuecomment-2848731381 @ViliusS, @ppkarwasz, I think we better proceed with the current state of this PR – I've approved it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [I] Allow disabling the optimization for filling the stack traces [logging-log4j2]
ppkarwasz commented on issue #3639: URL: https://github.com/apache/logging-log4j2/issues/3639#issuecomment-2848815544 @vy, sure! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [I] Allow disabling the optimization for filling the stack traces [logging-log4j2]
ppkarwasz closed issue #3639: Allow disabling the optimization for filling the stack traces URL: https://github.com/apache/logging-log4j2/issues/3639 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adjust GcpLayout JSON to latest format [logging-log4j2]
ppkarwasz merged PR #3586: URL: https://github.com/apache/logging-log4j2/pull/3586 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Disable (temporarily) Dependabot updates [logging-log4j2]
ppkarwasz commented on PR #3624: URL: https://github.com/apache/logging-log4j2/pull/3624#issuecomment-2848821155 > what is long-term solution to this problem? The main problem to solve is how to trigger a build on the commit with the changelog. I haven't test any of these, which is why I call the "longterm" solutions: 1. We could try to trigger the build using `workflow_run` like in [`develocity-publish-build-scans.yaml`](https://github.com/apache/logging-log4j2/blob/2.x/.github/workflows/develocity-publish-build-scans.yaml). 2. We could try to trigger the build, when the PR is approved. 3. We could try using labels. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adapt `merge-dependabot` to RTC [logging-log4j2]
ppkarwasz commented on PR #3603: URL: https://github.com/apache/logging-log4j2/pull/3603#issuecomment-2848823194 Maybe it is easier to write it in TypeScript and reference it from `logging-parent`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enforce RTC [logging-log4j-samples]
ppkarwasz merged PR #320: URL: https://github.com/apache/logging-log4j-samples/pull/320 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [I] DefaultRolloverStrategy creating rolled over log files more than the configured limit [logging-log4j2]
github-actions[bot] closed issue #3475: DefaultRolloverStrategy creating rolled over log files more than the configured limit URL: https://github.com/apache/logging-log4j2/issues/3475 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
