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

Added enhancement to slack publish with build failure cause #1819

Merged

Conversation

prudhvigodithi
Copy link
Member

@prudhvigodithi prudhvigodithi commented Mar 24, 2022

Signed-off-by: pgodithi pgodithi@amazon.com

Description

With this PR, the slack build failure message will be enhanced with actually error.
Example while distribution-build-opensearch with manifest 2.0.0/opensearch-2.0.0.yml

MESSAGE=2022-03-24 01:47:24 ERROR    Error building job-scheduler
 retry with: ./build.sh manifests/2.0.0/opensearch-2.0.0.yml --component job-scheduler --snapshot
2022-03-24 01:48:51 ERROR    Error building job-scheduler
 retry with: ./build.sh manifests/2.0.0/opensearch-2.0.0.yml --component job-scheduler --snapshot

Issues Resolved

#1672

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: pgodithi <pgodithi@amazon.com>
@prudhvigodithi prudhvigodithi requested a review from a team as a code owner March 24, 2022 12:24
Signed-off-by: pgodithi <pgodithi@amazon.com>
@codecov-commenter
Copy link

codecov-commenter commented Mar 24, 2022

Codecov Report

Merging #1819 (c913b22) into main (6a06d7d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               main    #1819   +/-   ##
=========================================
  Coverage     94.53%   94.53%           
  Complexity       19       19           
=========================================
  Files           176      177    +1     
  Lines          3622     3623    +1     
  Branches         27       27           
=========================================
+ Hits           3424     3425    +1     
  Misses          194      194           
  Partials          4        4           
Impacted Files Coverage Δ
tests/jenkins/jobs/PublishNotification_Jenkinsfile 100.00% <ø> (ø)
tests/jenkins/jobs/BuildFailureMessage_Jenkinsfile 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 6a06d7d...c913b22. Read the comment docs.

Signed-off-by: pgodithi <pgodithi@amazon.com>
Signed-off-by: pgodithi <pgodithi@amazon.com>
Signed-off-by: pgodithi <pgodithi@amazon.com>
@prudhvigodithi prudhvigodithi marked this pull request as draft March 24, 2022 15:45
@prudhvigodithi prudhvigodithi requested review from peterzhuamazon and removed request for peterzhuamazon March 24, 2022 15:45
Copy link
Member

@peterzhuamazon peterzhuamazon left a comment

Choose a reason for hiding this comment

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

@abhinavGupta16 do we want @prudhvigodithi to use this new way of testing scripts or using the old ways of lib-testers?

Comment on lines 12 to 15
message=message.replaceAll(",", "\n")
if(message.isEmpty()){
message="Build failed"
}
Copy link
Contributor

@abhinavGupta16 abhinavGupta16 Mar 24, 2022

Choose a reason for hiding this comment

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

Nit:

Suggested change
message=message.replaceAll(",", "\n")
if(message.isEmpty()){
message="Build failed"
}
if(message.isEmpty()){
message="Build failed"
} else {
message=message.replaceAll(",", "\n")
}

Copy link
Member Author

Choose a reason for hiding this comment

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

We want this to message.replaceAll(",", "\n") even-though message.isEmpty() is true as want to separate the Error messages one per line
Example slack output

MESSAGE=2022-03-24 15:30:20 ERROR    Error building job-scheduler
 retry with: ./build.sh manifests/2.0.0/opensearch-2.0.0.yml --component job-scheduler --snapshot
2022-03-24 15:31:41 ERROR    Error building job-scheduler
 retry with: ./build.sh manifests/2.0.0/opensearch-2.0.0.yml --component job-scheduler --snapshot

Copy link
Contributor

Choose a reason for hiding this comment

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

if message is an empty string, what will .replaceAll replace?

def performance_log = currentBuild.rawBuild.getLog(Integer.MAX_VALUE).join('\n')
performance_log.eachLine() { line ->
if (line.matches(".*$ERROR_STRING.*")) {
message+=(line+delimiter)
Copy link
Contributor

Choose a reason for hiding this comment

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

This message can be long if there are a log of errors. Let's make it a StringBuffer or a StringBuilder instead

Copy link
Member Author

@prudhvigodithi prudhvigodithi Mar 25, 2022

Choose a reason for hiding this comment

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

Thats a good point, Added Reader reading encoded character and pushed in my latest commit.

args.manifest,
"${args.icon}",
"JOB_NAME=${JOB_NAME}",
"BUILD_NUMBER=[${BUILD_NUMBER}]",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use an array here? Can there be multiple build numbers in one notification?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is part of already existing code, I have just made more readable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohh.... I see. I wonder why is it an array though

def message = ""
String delimiter = ","
def performance_log = currentBuild.rawBuild.getLog(Integer.MAX_VALUE).join('\n')
performance_log.eachLine() { line ->
Copy link
Member

Choose a reason for hiding this comment

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

I'm wonder how long this normally takes since it's iterating through the log? This could be inefficient if the log is long. Could we possibly read through the bottom of the log where the error is generally close to?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @zelinh, currentBuild.rawBuild.getLog(Integer.MAX_VALUE) we can add a value in place of Integer.MAX_VALUE, but this will get only last few lines of that value, and there are high chances we miss the ERROR message in those lines, so which is why I have added all lines and go over each line in running log, also this is part of post() script, which runs as part of same pipeline, so it should go over all the currentBuild log to get that ERROR string.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to read the file line by line and process it, keeping in mind, string creation is an expensive operation? This goes through the entire file, reads and creates a string out of it with line breaks. It then goes over it again to filter error messages to a new string. WDTY?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes right now this code reads line by line, and filters for the log error message, I'm looking for a buffered stream here, as explained above this is part of Same pipeline as post() call, so we need to read all lines each line as part of same pipeline.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but it currently goes through all the lines twice and makes 2 strings. Can we change it to only once?

Copy link
Member Author

@prudhvigodithi prudhvigodithi Mar 25, 2022

Choose a reason for hiding this comment

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

@abhinavGupta16 check my latest commit, this time I have added Reader buffer.

Signed-off-by: pgodithi <pgodithi@amazon.com>
Signed-off-by: pgodithi <pgodithi@amazon.com>
Signed-off-by: pgodithi <pgodithi@amazon.com>
Signed-off-by: pgodithi <pgodithi@amazon.com>
Signed-off-by: pgodithi <pgodithi@amazon.com>
Signed-off-by: pgodithi <pgodithi@amazon.com>
Signed-off-by: pgodithi <pgodithi@amazon.com>
Signed-off-by: pgodithi <pgodithi@amazon.com>
@abhinavGupta16
Copy link
Contributor

abhinavGupta16 commented Mar 24, 2022

@abhinavGupta16 do we want @prudhvigodithi to use this new way of testing scripts or using the old ways of lib-testers?

We'll need libtesters

Signed-off-by: pgodithi <pgodithi@amazon.com>
Signed-off-by: pgodithi <pgodithi@amazon.com>
Signed-off-by: pgodithi <pgodithi@amazon.com>
Signed-off-by: pgodithi <pgodithi@amazon.com>
Reader performance_log = currentBuild.getRawBuild().getLogReader()
String logContent = IOUtils.toString(performance_log)
performance_log.close()
performance_log = null
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Why do we reassign it to null?

Copy link
Member Author

Choose a reason for hiding this comment

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

Stream object will be eligible for garbage collection

Signed-off-by: pgodithi <pgodithi@amazon.com>
Signed-off-by: pgodithi <pgodithi@amazon.com>
Signed-off-by: pgodithi <pgodithi@amazon.com>
Signed-off-by: pgodithi <pgodithi@amazon.com>
Signed-off-by: pgodithi <pgodithi@amazon.com>
Signed-off-by: pgodithi <pgodithi@amazon.com>
Signed-off-by: pgodithi <pgodithi@amazon.com>
Signed-off-by: pgodithi <pgodithi@amazon.com>
@prudhvigodithi prudhvigodithi marked this pull request as ready for review March 29, 2022 00:40
Signed-off-by: pgodithi <pgodithi@amazon.com>
Copy link
Member

@gaiksaya gaiksaya left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks!

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.

6 participants