Skip to content

[JENKINS-54128] More efficient implementation of getLogFile that just uses the existing file when possible #118

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.

Sign up for GitHub

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

Merged
dwnusbaum merged 6 commits into from
Nov 9, 2018
Merged

[JENKINS-54128] More efficient implementation of getLogFile that just uses the existing file when possible #118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
58399b8
More efficient implementation of getLogFile that just uses the existi…
jglick Nov 8, 2018
660583a
Use writeLogTo helper method.
dwnusbaum Nov 8, 2018
f8f429f
Merge branch 'master' into JENKINS-54128
jglick Nov 8, 2018
ba8691d
Test for getLogFile which incidentally caught a bug in getLogInputStr…
jglick Nov 8, 2018
4a4256b
For the copy-from-cloud case, clearer to use a nontraditional filename.
jglick Nov 8, 2018
6293f9b
Implementing LogStorage.getLogFile from https://github.com/jenkinsci/…
jglick Nov 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
<parent>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>plugin</artifactId>
<version>3.25</version>
<version>3.28</version>
<relativePath />
</parent>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
Expand Down Expand Up @@ -82,7 +82,7 @@
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-api</artifactId>
<version>2.31</version>
<version>2.32-rc862.5f9ac65fd5f2</version> <!-- TODO https://github.com/jenkinsci/workflow-api-plugin/pull/82 -->
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
Expand Down
14 changes: 2 additions & 12 deletions src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
Expand Down Expand Up @@ -1019,7 +1018,7 @@ private final class NodePrintListener implements GraphListener.Synchronous {
@Override public InputStream getLogInputStream() throws IOException {
// Inefficient but probably rarely used anyway.
ByteArrayOutputStream baos = new ByteArrayOutputStream();
writeLogTo(getLogText()::writeLogTo, baos);
writeLogTo(getLogText()::writeRawLogTo, baos);
Copy link
Member

@dwnusbaum dwnusbaum Nov 9, 2018

Choose a reason for hiding this comment

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

Yikes, caused by my changes in #114. Good catch!

return new ByteArrayInputStream(baos.toByteArray());
}

Expand Down Expand Up @@ -1081,16 +1080,7 @@ private void writeLogTo(WriteMethod method, OutputStream os) throws IOException
@Deprecated
@Override public File getLogFile() {
LOGGER.log(Level.WARNING, "Avoid calling getLogFile on " + this, new UnsupportedOperationException());
Copy link
Member

@dwnusbaum dwnusbaum Nov 8, 2018

Choose a reason for hiding this comment

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

Given the number of existing callers, should we only log the stack trace at FINE level?

Copy link
Member Author

@jglick jglick Nov 8, 2018

Choose a reason for hiding this comment

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

Well…we only know about those callers because it is at WARNING. And this would be important e.g. for getting sentry.io to show violations in Evergreen.

Copy link
Member

@svanoort svanoort Nov 8, 2018

Choose a reason for hiding this comment

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

Isn't this potentially going to generate a log of logspam though?

Copy link
Member

@svanoort svanoort Nov 8, 2018

Choose a reason for hiding this comment

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

Also, I don't see that (at least on current Jenkins master branch) that Run.getLogFile is deprecated, which makes it feel very odd to me that we're deprecated its override in WorkflowJob

Copy link
Member Author

@jglick jglick Nov 8, 2018

Choose a reason for hiding this comment

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

Isn't this potentially going to generate a log of logspam though?

Potentially. Note that the log call code was already there—not touched by this PR.

I don't see that (at least on current Jenkins master branch) that Run.getLogFile is deprecated

Yup, on my to-do list.

Copy link
Member

@svanoort svanoort Nov 9, 2018

Choose a reason for hiding this comment

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

Why? While it may be satisfying to simple signal "stop using this API" by making it log a warning every time, for a user this looks like the Workflow Job plugin was broken by the update that added the logging - we've seen this before with "cosmetic" log warnings. Just not great optics or user experience.

Being able to point to deprecation of the extension point in core and an appropriate waiting period gives us more of a leg to stand on here.

Copy link
Member

@timja timja Nov 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

so what do I call instead?
I'm making changes to the build-failure-analyzer plugin, and saw my logs being spammed here.
but there's no javadoc explaining what the replacement / another solution is?

It does look removeable as a whole potentially as all it does is get a file name off it but not sure the full implications

https://github.com/hmcts/build-failure-analyzer-plugin/blob/6176af08f11355b427a1db1a1c44c66dafe3de02/src/main/java/com/sonyericsson/jenkins/plugins/bfa/model/BuildLogFailureReader.java#L66

Copy link
Member Author

@jglick jglick Nov 30, 2018

Choose a reason for hiding this comment

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

@timja Any log-related methods other than getLogFile. getLogText is the basic one. In your case I would suggest simply

-String currentFile = build.getLogFile().getName();
+String currentFile = "log";

Copy link
Member

@timja timja Nov 30, 2018

Choose a reason for hiding this comment

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

Will give it a go later on thanks 👍

Copy link
Member Author

@jglick jglick Apr 2, 2019

Choose a reason for hiding this comment

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

I don't see that (at least on current Jenkins master branch) that Run.getLogFile is deprecated

jenkinsci/jenkins#3963

try {
File f = File.createTempFile("deprecated", ".log", getRootDir());
f.deleteOnExit();
try (OutputStream os = new FileOutputStream(f)) {
getLogText().writeRawLogTo(0, os);
}
return f;
} catch (IOException x) {
throw new RuntimeException(x);
}
return LogStorage.of(asFlowExecutionOwner()).getLogFile(this, !isLogUpdated());
}

static void alias() {
Expand Down
10 changes: 10 additions & 0 deletions src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@
import org.xml.sax.SAXException;

import javax.annotation.Nonnull;
import org.apache.commons.io.IOUtils;

public class WorkflowRunTest {

Expand Down Expand Up @@ -503,4 +504,13 @@ private void assertCulprits(WorkflowRun b, String... expectedIds) throws IOExcep
r.assertLogContains(message, r.buildAndAssertSuccess(p));
}

@Issue("JENKINS-54128")
@SuppressWarnings("deprecation")
@Test public void getLogFile() throws Exception {
WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p");
p.setDefinition(new CpsFlowDefinition("echo 'sample text'", true));
WorkflowRun b = r.buildAndAssertSuccess(p);
assertEquals(IOUtils.toString(b.getLogInputStream()), FileUtils.readFileToString(b.getLogFile()));
}

}