[JENKINS-52692,JENKINS-38313] - External task logging API #3557

oleg-nenashev · 2018-07-23T06:40:59Z

See JENKINS-52692. This pull request is a WiP set of changes for the External Build Loggin JEP which will be submitted soon. It includes the External Logging Prototype created back in 2016 together with @xyan0607 + new API changes I am doing to reflect the current design.

Downstream pull requests:

Proposed changelog entries

Entry 1: Issue, Human-readable Text
...

Submitter checklist

JIRA issue is well described
Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
* Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
Appropriate autotests or explanation to why this change has no tests
For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

…bmissions from Elasticsearch

Hide launcher complexity from plugin.

…-task-logging-poc # Conflicts: # core/src/main/java/hudson/model/AbstractProject.java # core/src/main/java/hudson/model/Job.java # core/src/main/java/hudson/model/Run.java

# Conflicts: # core/src/main/java/hudson/model/Job.java # core/src/main/java/hudson/model/Run.java

…sisted for Runs + extend API

…es correctly

…I plugin

…tation

…Method

jglick

Some initial comments from a brief look. I expect to go back over this in detail.

jglick · 2018-07-23T17:36:03Z

core/src/main/java/hudson/console/ConsoleNote.java

@@ -292,7 +292,7 @@ public static void skip(DataInputStream in) throws IOException {
    }

    private static final long serialVersionUID = 1L;
-
+    


Revert please.

jglick · 2018-07-23T18:55:09Z

core/src/main/java/jenkins/model/logging/Loggable.java

+    /**
+     * Returns charset to be used.
+     * New implementations are recommended to use {@code UTF-8}-only (default),
+     * but the method can be overridden by legacy implementations.


I feel that this unnecessarily complicates the system, adding to the conceptual burden. There are no “legacy implementations” of a brand-new API. If you are referring to freestyle build logs, now is the time to fix them, like we did for Pipeline in JEP-206.

Cc @Casz who also wanted to get rid of non-UTF-8 logging iirc

I concur with jesse.

Current design: https://github.com/jenkinsci/jep/tree/master/jep/207#encoding-of-log-files

Currently Jenkins does not set limitations for encoding while doing logging. Any charsets may be used on agent and master sides, and it is hard to manage them. Although it is expected that all logs eventually switch to UTF-8 (see the JEP-206 proposal for Pipeline), in meantime external logging may be performed in different encodings. * Loggable implementations can define the charset to be used * Logging Method and Logging Browser implementations may implement support of charsets or reject them, it is up to the implementation * If the implementation does not support a charset, `ExternalLoggingMethodLocator` can skip the logging method

I believe this is a more conservative approach. I do not want to fix Freestyle logging in this change, because it may introduce serious regressions.

If you prefer, I could add a supportsNonUTF8() method (defaults to false) so that LoggingMethodLocator checks support automatically.

I do not want to fix Freestyle logging in this change, because it may introduce serious regressions.

Well, the same could be said for JEP-206, but my feeling was that it was worth risking regressions in the interest of making the system design simpler and clearer in the long run.

(see #3231)

jglick · 2018-07-23T18:56:21Z

core/src/main/java/jenkins/model/logging/Loggable.java

+     * @return Temporary directory or {@code null} if not defined
+     */
+    @CheckForNull
+    default File getTmpDir() {


What would override this and to what value? Seems YAGNI

Yes, it's YAGNI after the today's refactoring

jglick · 2018-07-23T18:58:35Z

core/src/main/java/jenkins/model/logging/LogBrowser.java

+import java.util.List;
+
+/**
+ * Defines how the logs should be browsed.


I do not think it makes sense to split this from LogMethod. While for certain implementations of external log storage there may be multiple potential browsers that we would like to allow users to select independently, I would not expect that to be universally true, and at worst such implementations could define their own extension point.

It seems that the only actual extension point so far is LoggingMethodLocator, so what is the purpose of separating these interfaces?

Please comment in JEP instead: https://github.com/jenkinsci/jep/tree/master/jep/207#why-loggingmethod-and-logbrowser-are-separated

jglick · 2018-07-23T18:59:11Z

core/src/main/java/hudson/model/AbstractProject.java

@@ -2129,7 +2130,7 @@ public void setCustomWorkspace(String customWorkspace) throws IOException {
        this.customWorkspace= Util.fixEmptyAndTrim(customWorkspace);
        save();
    }
-
+    


Revert please.

jglick · 2018-07-23T19:02:24Z

core/src/main/java/jenkins/model/logging/LogHandler.java

+    }
+
+    @Exported
+    public String getId() {


What is this used for? I would drop it. Stapler exporting already specifies _class where required.

jglick · 2018-07-23T19:03:06Z

core/src/main/java/jenkins/model/logging/LogBrowser.java

+    /**
+     * Gets log for a part of the object.
+     * @param stepId Identifier of the step to be displayed.
+     *               It may be Pipeline step or other similar abstraction


Why does this need to be defined here? Seems like it belongs only in external-logging-api.

"Steps" may exist for custom logging implementations , eventually not only for Pipeline. So I would prefer to keep API here at lower level

jglick · 2018-07-23T19:06:08Z

core/src/main/java/jenkins/model/logging/impl/CloseableStreamBuildListener.java

+ * @since TODO
+ */
+@Restricted(Beta.class)
+public class CloseableStreamBuildListener extends StreamBuildListener implements Closeable {


Huh? StreamTaskListener is already Closeable.

Yes, to be deleted at all

jglick · 2018-07-23T19:07:06Z

core/src/main/java/jenkins/model/logging/impl/StreamLoggingMethod.java

+
+    /**
+     * Defines an additional Console Log Filter to be used with the logging method.
+     * This filter may be also used for log redirection and multi-reporting in very custom cases.


jglick · 2018-07-23T19:07:17Z

core/src/main/java/jenkins/model/logging/impl/StreamLoggingMethod.java

+        final Job<?, ?> project = build.getParent();
+
+        // Project specific log filters
+        if (project instanceof BuildableItemWithBuildWrappers && build instanceof AbstractBuild) {


This looks like a failure of abstraction.

It is. But this is an exisiting code, just moved it. Happy to add TODO and follow-up JIRA

I see, did not notice this was moved out of Run.

oleg-nenashev · 2018-07-25T16:09:19Z

@jglick if we merge LogBrowser and LoggingMethod, would you be fine with the LogStorage name?

jglick · 2018-07-25T17:38:29Z

if we merge LogBrowser and LoggingMethod

(At least in core—not necessarily in external-logging-api.)

would you be fine with the LogStorage name?

Yes that sounds sensible. In such a case jenkinsci/workflow-api-plugin#17 might be better renamed to PipelineLogStorage. It is related but not identical.

jglick

We discussed some planned changes. Taking off my review queue for now.

oleg-nenashev · 2018-07-31T10:44:25Z

The fact that the Incrementals notification is green when deployment fails is totally confusing. There is a ticket for that somewhere

Skipping deployment as no artifacts were found with the expected path, typically due to a PR merge build not up to date with its base branch: https://ci.jenkins.io/job/Core/job/jenkins/job/PR-3557/6/artifact/**/*-rc*.f56ea7aafcaa/*-rc*.f56ea7aafcaa*/*zip*/archive.zip

…-task-logging

…ation to External logging API

…-task-logging

batmat · 2019-04-04T13:58:19Z

Given this PR is conflicted, and Oleg has himself marked this as stalled, I am going to close it.

Oleg, please obviously feel free to reopen anytime if you feel like it.

Thanks!

Xing Yan and others added 17 commits September 17, 2016 11:42

Initial interface of LoggingMethod.

4c6355b

Skeleton implementation of the remote logging extension point

28386a5

HACK: Get rid of the special symbol in order to be able to process su…

79ef4c0

…bmissions from Elasticsearch

Polish Javadoc a bit

69a7ef4

Hide launcher complexity from plugin.

5c287b3

Update LoggingDefinitionLauncherWrapper.java

febb8bf

Update LoggingMethod.java

b0d8e64

Merge pull request #5 from xyan0607/jw-hackaton-external-logging

948db76

Hide launcher complexity from plugin.

Save the progress

5224058

Merge commit 'ac05680bb6eba6efc8a825ba59fda6fbad753d22' into external…

1c37b73

…-task-logging-poc # Conflicts: # core/src/main/java/hudson/model/AbstractProject.java # core/src/main/java/hudson/model/Job.java # core/src/main/java/hudson/model/Run.java

Merge branch 'master' into external-task-logging

5ffea98

# Conflicts: # core/src/main/java/hudson/model/Job.java # core/src/main/java/hudson/model/Run.java

[JENKINS-38313] - Facelift the implementation

e64d206

[JENKINS-38313] - Add the LogBrowser stub

d5c4651

[JENKINS-38313] - References to log storages and browsers are now per…

02fd209

…sisted for Runs + extend API

Cleanup FindBugs + Beta annotations

86d8e43

Revert custom patches

21aae74

[JENKINS-38313] - Adjust initialization logic so that Pipeline Operat…

d26d38a

…es correctly

oleg-nenashev self-assigned this Jul 23, 2018

oleg-nenashev added the work-in-progress The PR is under active development, not ready to the final review label Jul 23, 2018

oleg-nenashev added 3 commits July 23, 2018 14:49

[JENKINS-38313] - Move Launcher wrapping logic to External Logging AP…

5f6213e

…I plugin

[JENKINS-38313] - Refactor LoggingMethod API to simplify the implemen…

da4441a

…tation

[JENKINS-38313] - Align TaskListener and BuildListener API in Logging…

b193607

…Method

jglick self-requested a review July 23, 2018 17:35

jglick reviewed Jul 23, 2018

View reviewed changes

jglick self-requested a review July 23, 2018 19:08

oleg-nenashev added 2 commits July 23, 2018 22:41

[JENKINS-38313] - CloseableStreaMBuildListener is not required

a2e0317

[JENKINS-38313] - Cleanup formatting, imports and licenses

48d03e0

oleg-nenashev mentioned this pull request Jul 25, 2018

Functionality to delete logs. #3545

Closed

4 tasks

jglick reviewed Jul 25, 2018

View reviewed changes

This was referenced Jul 26, 2018

[JENKINS-52693] - External Build Logging. Reference Implementation for Elasticsearch jenkinsci/external-logging-elasticsearch-plugin#1

Merged

External Logging API Implementation. v1 jenkinsci/external-logging-api-plugin#1

Merged

Add TODO from @jglick, unrestrict API

f56ea7a

oleg-nenashev added 5 commits July 31, 2018 12:45

Merge commit '25da480fab1da7d2a01ab68a56faf00cae0c23b1' into external…

ac9633a

…-task-logging

Merge branch 'master' into external-task-logging

e15bdc1

[JENKINS-38313] - Move LogBrowser#getFile() default caching implement…

62024d7

…ation to External logging API

[JENKINS-38313] - Introduce API For Deleting logs

7ebcb15

[JENKINS-38313] - Adapt NoopLogFile to the new API

70e154e

This was referenced Aug 3, 2018

[JENKINS-38313] - Introduce LogStorage API in the core #3575

Closed

External Logging API: Align with new Core version, fix Findbugs jenkinsci/external-logging-api-plugin#2

Merged

Merge commit '7b2698db89183b4d701107485db61eb36442ac25' into external…

f996af4

…-task-logging

oleg-nenashev mentioned this pull request Aug 31, 2018

[JENKINS-17116] - When aborting a build, wait up to 2min for process termination #3414

Merged

oleg-nenashev added stalled The PR is reasonable and might be merged, but it is no longer active. It can be taken over by other and removed work-in-progress The PR is under active development, not ready to the final review labels Feb 1, 2019

Merge branch 'master' into external-task-logging

198fc7a

oleg-nenashev mentioned this pull request Apr 2, 2019

Deprecating Run.getLogFile #3963

Merged

batmat added the unresolved-merge-conflict There is a merge conflict with the target branch. label Apr 4, 2019

batmat closed this Apr 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-52692,JENKINS-38313] - External task logging API #3557

[JENKINS-52692,JENKINS-38313] - External task logging API #3557

oleg-nenashev commented Jul 23, 2018 •

edited

Loading

jglick left a comment

jglick Jul 23, 2018

jglick Jul 23, 2018

oleg-nenashev Jul 24, 2018

jetersen Jul 25, 2018

oleg-nenashev Jul 25, 2018

jglick Jul 25, 2018

jglick Jul 31, 2018

jglick Jul 23, 2018

oleg-nenashev Jul 23, 2018

jglick Jul 23, 2018

oleg-nenashev Jul 25, 2018

jglick Jul 23, 2018

jglick Jul 23, 2018

jglick Jul 23, 2018

oleg-nenashev Jul 24, 2018

jglick Jul 23, 2018

oleg-nenashev Jul 23, 2018

jglick Jul 23, 2018

jglick Jul 23, 2018

oleg-nenashev Jul 23, 2018

jglick Jul 25, 2018

oleg-nenashev commented Jul 25, 2018

jglick commented Jul 25, 2018

jglick left a comment

oleg-nenashev commented Jul 31, 2018 •

edited

Loading

batmat commented Apr 4, 2019

		@@ -292,7 +292,7 @@ public static void skip(DataInputStream in) throws IOException {
		}

		private static final long serialVersionUID = 1L;

[JENKINS-52692,JENKINS-38313] - External task logging API #3557

[JENKINS-52692,JENKINS-38313] - External task logging API #3557

Conversation

oleg-nenashev commented Jul 23, 2018 • edited Loading

Proposed changelog entries

Submitter checklist

Desired reviewers

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev commented Jul 25, 2018

jglick commented Jul 25, 2018

jglick left a comment

Choose a reason for hiding this comment

oleg-nenashev commented Jul 31, 2018 • edited Loading

batmat commented Apr 4, 2019

oleg-nenashev commented Jul 23, 2018 •

edited

Loading

oleg-nenashev commented Jul 31, 2018 •

edited

Loading