Reduce the GitClient instances creation for CI Visibility pipelines#240
Conversation
drodriguezhdez
added
the
changelog/Fixed
| this.gitMessage = gitCommitAction.getMessage(); | ||
| this.gitAuthorName = gitCommitAction.getAuthorName(); | ||
| this.gitAuthorEmail = gitCommitAction.getAuthorEmail(); | ||
| this.gitAuthorDate = gitCommitAction.getAuthorDate(); | ||
| this.gitCommitterName = gitCommitAction.getCommitterName(); | ||
| this.gitCommitterEmail = gitCommitAction.getCommitterEmail(); | ||
| this.gitCommitterDate = gitCommitAction.getCommitterDate(); |
There was a problem hiding this comment.
This code is duplicated here and in lines 286-294. I would extract it to a function.
| GitClient gitClient = null; | ||
| if(isValidCommit(gitCommit) || isValidRepositoryURL(this.gitUrl)) { | ||
| // Create a new Git client is a very expensive operation. | ||
| // Avoid creating Git clients as much as possible. | ||
| gitClient = GitUtils.newGitClient(run, listener, envVars, this.nodeName, this.workspace); | ||
| } |
There was a problem hiding this comment.
| GitClient gitClient = null; | |
| if(isValidCommit(gitCommit) || isValidRepositoryURL(this.gitUrl)) { | |
| // Create a new Git client is a very expensive operation. | |
| // Avoid creating Git clients as much as possible. | |
| gitClient = GitUtils.newGitClient(run, listener, envVars, this.nodeName, this.workspace); | |
| } | |
| if(!isValidCommit(gitCommit) && !isValidRepositoryURL(this.gitUrl)) { | |
| return; | |
| } | |
| GitClient gitClient = GitUtils.newGitClient(run, listener, envVars, this.nodeName, this.workspace); |
| final boolean commitInfoAlreadyCreated = commitAction != null && commitAction.getCommit() != null && commitAction.getCommit().equals(gitCommit); | ||
| final boolean repoInfoAlreadyCreated = repositoryAction != null && repositoryAction.getRepositoryURL() != null && repositoryAction.getRepositoryURL().equals(gitUrl); |
There was a problem hiding this comment.
There boolean conditions are duplicated here and in BuildData. It might be worth extracting to a method.
| } catch (Exception e) { | ||
| LOGGER.fine("Unable to build RepositoryInfo. Error: " + e); | ||
| LOGGER.info("Unable to build RepositoryInfo. Error: " + e); | ||
| return null; |
There was a problem hiding this comment.
Why aren't you returning RepositoryInfo.EMPTY_REPOSITORY_INFO here?
There was a problem hiding this comment.
For errors, we return null to trigger again the logic in case the repository info was calculated again. By other hand, the case where we actually know that the info could be calculated but there is no valid info for us, we return the empty repository info.
| GitCommitAction gitCommitAction = run.getAction(GitCommitAction.class); | ||
| GitRepositoryAction gitRepositoryAction = run.getAction(GitRepositoryAction.class); |
There was a problem hiding this comment.
I understand the strategy here, but I don't understand where's the code that calls .addAction with the git client. Can you point me to that place?
There was a problem hiding this comment.
Sure. The methods are GitUtils.buildGitRepositoryAction(...) and GitUtils.buildGitCommitAction(...)
| public static long currentTimeMillis() { | ||
| if(!LOGGER.isLoggable(Level.FINE)){ | ||
| return -1L; | ||
| } | ||
|
|
||
| return System.currentTimeMillis(); | ||
| } | ||
|
|
||
| public static void log(String msg, long start, long end) { | ||
| if(start == -1L || end == -1L){ | ||
| return; | ||
| } |
There was a problem hiding this comment.
I think the interface of this class can be improved. It's a bit weird having to rely on a wrapped version of curentTimeMillis that returns a sentinel value. What about something like this?
public class DatadogAudit {
public static void log(String msg);
public static DatadogAuditStopwatch startWatch();
}
public class DatadogAuditStopwatch {
public void stop(String msg) {
long duration = System.currentTimeMillis() - this.start;
if(duration > 10){
DatadogAudit.logFine(msg +" [duration: "+duration+" ms, start: " + start + ", end: " + end+"]");
}
}
}Or something like this:
public static void log(String msg, long start, long end) {
if(!LOGGER.isLoggable(Level.FINE)){
return;
}
[...]
2 participants
Requirements for Contributing to this repository
What does this PR do?
This PR adds performance improvements, especially related to
GitClientinstances:DatadogStepListenerlogic now is only executed if CI Visibility is enabled.GitClientinstance is created.GIT_URLorGIT_COMMIT, noGitClientis created.GIT_URLandGIT_COMMIT, no newGitClientinstance is created after.DatadogAuditis used to send logs with the duration of certain methods. This can be opt-in by the user in the Jenkins log manager.Context
We use the
GitClientinstance to extract the details about a commit (author, message, etc) and the default branch.The creation of the
GitClientinstance is a very expensive operation.Description of the Change
Alternate Designs
Possible Drawbacks
Verification Process
Additional Notes
Release Notes
Review checklist (to be filled by reviewers)
changelog/label attached. If applicable it should have thebackward-incompatiblelabel attached.do-not-merge/label attached.kind/andseverity/labels attached at least.