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

improve log by adding the jobname as context #41

Merged
merged 1 commit into from
Jan 31, 2021

Conversation

theKBro
Copy link
Contributor

@theKBro theKBro commented Jan 30, 2021

No description provided.

@codecov
Copy link

codecov bot commented Jan 30, 2021

Codecov Report

Merging #41 (15c3666) into master (71e42eb) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
+ Coverage   98.98%   99.00%   +0.01%     
==========================================
  Files          23       23              
  Lines         198      201       +3     
  Branches       19       19              
==========================================
+ Hits          196      199       +3     
  Misses          1        1              
  Partials        1        1              
Impacted Files Coverage Δ
...nkins/buildhistorymanager/BuildHistoryManager.java 100.00% <100.00%> (ø)
...epanik/jenkins/buildhistorymanager/model/Rule.java 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71e42eb...15c3666. Read the comment docs.

@@ -40,23 +40,26 @@ public BuildHistoryManager(List<Rule> rules) {
*/
@Override
public synchronized void perform(Job<?, ?> job) throws IOException, InterruptedException {
String uniquePerformName = job.getFullName();
Copy link
Member

Choose a reason for hiding this comment

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

full name or display name ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

full name, else you cannot distinguish two projects with the same display name in different folders

}

Run<?, ?> run = job.getLastCompletedBuild();
// for each completed build...
while (run != null) {
LOG.info("Processing build #" + run.getNumber());
LOG.info(uniquePerformName + ": Processing build #" + run.getNumber());
Copy link
Member

Choose a reason for hiding this comment

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

since this method is synchronized I don't think there is value to pass this into each log

Copy link
Contributor Author

@theKBro theKBro Jan 31, 2021

Choose a reason for hiding this comment

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

synchronized only for each instance's execution or am I wrong?
so there is the possibility that two different instances/projects are analyzed in parallel

Copy link
Member

Choose a reason for hiding this comment

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

but then you have two separate logs - is that right?

I'm not good at case which you currently having so that's why I'm asking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no this is not the console output of the current build, this is part of the system log.
if you want to log to the console output you need a RunListener and call getLogger() on that

Copy link
Contributor Author

@theKBro theKBro Jan 31, 2021

Choose a reason for hiding this comment

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

image

this is what the log looks like with the changes and 2 projects only

and the configuration for the log is

image

@@ -17,7 +17,7 @@ public static Job buildSampleJob() {

public static Job buildSampleJob(Run lastBuild) {
Job job = mock(Job.class);

when(job.getFullName()).thenReturn("sampleJob");
Copy link
Member

Choose a reason for hiding this comment

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

why if you don't validate it

Copy link
Contributor Author

@theKBro theKBro Jan 31, 2021

Choose a reason for hiding this comment

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

if nothing is configured getFullName() returns null which will result in the code crashing

Copy link
Member

Choose a reason for hiding this comment

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

where?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On concatinating the log output.
Besides that in the "real world" this is never null

@damianszczepanik damianszczepanik merged commit 251ca7b into jenkinsci:master Jan 31, 2021
@damianszczepanik
Copy link
Member

@theKBro can you share the configuration for this - I see that I haven't tested the plugin against such case :(

@theKBro
Copy link
Contributor Author

theKBro commented Jan 31, 2021

just create multiple jobs, I did not do anything special here

@theKBro theKBro deleted the improve_log branch January 31, 2021 14:06
@damianszczepanik
Copy link
Member

so this change satisfy Jenkins global log but project log where the plugin runs is fine even without this change?

@damianszczepanik
Copy link
Member

then I don't see valuable of having this change merged as Jenkins log is rather for debugging plugin issue and problem with configuration can be found at particular job execution even without this change

@theKBro
Copy link
Contributor Author

theKBro commented Jan 31, 2021

Correct me if i'm wrong, but nothing is and was ever logged to job execution's console log? Or where can I find it?
Especially if this is executed hourly without any build context, where is this log located?

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