Possible issue with alarm "disable until" menu and "enabled" field in kafka and elasticsearch #3304
Comments
|
Well I am confused but it looks like maybe this is the intended behavior after all? So it could be the alarm logger needs to handle the case where enabled is a datestring and then if so, assume that means enabled is false? |
|
It may be more than what we fixed in the previous PR #3030 Could you describe how we recreate this issue? |
|
Here are the steps to re-create the issue:
The issue seems to me with how the enabled field in alarm server/kafka is handled. If you use the "Disable Until" feature it stuffs the date string into the enabled field. But from the code in the alarm logger, it seems like this field is expected to be a boolean. I am exploring a fix to the alarm logger serialization to convert date string to I also see It would be nice to see if anyone can re-produce this at their site since I am not using the latest version of phoebus or the services. Maybe this has all been cleaned up on the master branch |
|
@tynanford It looks like the bug you saw is the same one that we recently identified and fixed. I couldn't reproduce the issue. And I will review the exact troubleshooting documentation when I have some time. Tomorrow, please join the meeting where we will demonstrate the scenario together. You can evaluate the ALS-U "semi-production" version of all services through the pas-demo repository with our repository. Since the environment is local, you can delete it after testing. Additionally, I recommend rebuilding Phoebus with the patch code from this pull request: #3030. You only need to change one line if you prefer not to use the current master branch. See you later! |
|
@tynanford your observation is correct. With the introduction of the "disable until" functionality the enable field which previously only allowed boolean type now needed to support 2 types, The serialization and deserialization classes in the alarm server and application were updated to support the above change... Now, we should work on the short term fix that @jeonghanlee mentioned and update the alarm logger service and the alarm log table client to handle the situation where the It looks like the current mapping for the enable field is already a "string/keyword" we can update the serializer/deserializer for the POJOs and then allow both "true/false" and "date time"... we will have to update the POJO to change the enable filed from boolean to string.
I don't see any usage in the code where having the enable field as a boolean is helping so I think we should simply update the field and test. |
|
Hey great minds think alike! ;) I've been playing around with this patch and it seems to fix the issue like you said. Can test some more and create a PR In the future I could see it being nice to include the date string in the alarm log messages. So then people can see "Oh Joe disabled that alarm until next tuesday and then Sally disabled it even further out to Friday". Wonder in the next release if we could add a new field for disabled_until for config messages and make enabled only a boolean again diff --git a/services/alarm-logger/src/main/java/org/phoebus/alarm/logging/rest/AlarmLogMessage.java b/services/alarm-logger/src/main/java/org/phoebus/alarm/logging/rest/AlarmLogMessage.java
index ae96b1630..1e6e3d60e 100644
--- a/services/alarm-logger/src/main/java/org/phoebus/alarm/logging/rest/AlarmLogMessage.java
+++ b/services/alarm-logger/src/main/java/org/phoebus/alarm/logging/rest/AlarmLogMessage.java
@@ -11,6 +11,7 @@ import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import java.io.IOException;
import java.time.Instant;
+import java.time.LocalDateTime;
import java.time.ZoneId;
import java.time.format.DateTimeFormatter;
@@ -34,6 +35,7 @@ public class AlarmLogMessage {
private String user;
private String host;
private String command;
+ @JsonDeserialize(using = EnabledFieldDeserializer.class)
private boolean enabled;
public String getConfig() {
@@ -168,4 +170,25 @@ public class AlarmLogMessage {
return Instant.from(formatter.parse(p.getText()));
}
}
-}
\ No newline at end of file
+
+ public static class EnabledFieldDeserializer extends JsonDeserializer<Boolean> {
+
+ private static final DateTimeFormatter formatter = DateTimeFormatter.ISO_LOCAL_DATE_TIME;
+
+ @Override
+ public Boolean deserialize(JsonParser p, DeserializationContext ctxt) throws IOException, JsonProcessingException {
+ String text = p.getText();
+ try {
+ return Boolean.parseBoolean(text);
+ } catch (Exception e) {
+ try {
+ LocalDateTime.parse(text, formatter);
+ return false; // A date string means the alarm is disabled
+ } catch (Exception ex) {
+ // If all else fails, return false?
+ return false;
+ }
+ }
+ }
+ }
+} |
|
Ha! :) I have not been able to test my PR with your issue but if you could do that it would be great. It should show messages like "disabled until ...." |
|
Why do we work during the night? Thank you @shroffk and @tynanford to make the system much better than before! I like it! |
|
I am closing this PR #3314 cause I think we need to handle the new enable object better. It consists of the boolean and a time representation. I don't want to change the elastic mapping as far as possible |
|
Hi @shroffk , do you not like the deserialization idea for the logger API code? We lose the ability to show the timestamp in the alarm log table UI like your PR had, but it keeps the |
|
I think the deserialization is a good solution. |
We noticed at some point that the alarm log search stopped working. There was this error in the logger service which suggested the enabled field somehow has a date string instead of bool value:
I can reproduce this with phoebus v4.7.3 or the master branch by using either the calendar picker or pre-defined shelving_options dropdown. After disabling an alarm this way, elasticsearch has the date string in the enabled field. It also looks like kafka has the date string in enabled so it doesn't look like an issue in the alarm logger:
The alarm-server and alarm-logger service are running v4.7.3 but we tested with the master branch phoebus client. Is this working for anyone or has anyone else ran into this issue too?
The text was updated successfully, but these errors were encountered: