[GitHub] [skywalking] wu-sheng commented on a change in pull request #5509: Fix slack nullpointer issue

[GitBox <gi...@apache.org>]

wu-sheng commented on a change in pull request #5509:
URL: https://github.com/apache/skywalking/pull/5509#discussion_r490070952



##########
File path: oap-server/server-alarm-plugin/src/main/java/org/apache/skywalking/oap/server/core/alarm/provider/slack/SlackhookCallback.java
##########
@@ -77,31 +77,26 @@ public void doAlarm(List<AlarmMessage> alarmMessages) {
 
                 StringEntity entity;
                 try {
-
                     JsonObject jsonObject = new JsonObject();
                     JsonArray jsonElements = new JsonArray();
-
                     alarmMessages.forEach(item -> {
                         jsonElements.add(GSON.fromJson(
                             String.format(
                                 this.alarmRulesWatcher.getSlackSettings().getTextTemplate(), item.getAlarmMessage()
                             ), JsonObject.class));
                     });
-
                     jsonObject.add("blocks", jsonElements);
-
                     entity = new StringEntity(GSON.toJson(jsonObject), ContentType.APPLICATION_JSON);
-
                     post.setEntity(entity);
                     CloseableHttpResponse httpResponse = httpClient.execute(post);
                     StatusLine statusLine = httpResponse.getStatusLine();
                     if (statusLine != null && statusLine.getStatusCode() != HttpStatus.SC_OK) {
-                        log.error("send alarm to " + url + " failure. Response code: " + statusLine.getStatusCode());
+                        log.error("Send slack alarm to {} failure. Response code: {}", url , statusLine.getStatusCode());
                     }
                 } catch (UnsupportedEncodingException e) {
-                    log.error("Alarm to JSON error, " + e.getMessage(), e);
+                    log.error("Alarm to JSON error, {} ", e.getMessage(), e);
                 } catch (IOException e) {
-                    log.error("send alarm to " + url + " failure.", e);
+                    log.error("Send slack alarm to {} failure.", url , e);

Review comment:
       Actually, I think this should not change, but wechat callback should be changed.
   
   Take a look on the source codes
   ```
   public void error(Marker marker, String format, Object arg);
   
   public void error(String msg, Throwable t);
   ```
   
   If you want to print exception stack, then can't use the arguments for log text.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org