Closes #1285 - Agents regularly report status to config server #1377
Conversation
…back to warning/ok
Codecov Report
@@ Coverage Diff @@
## master #1377 +/- ##
============================================
+ Coverage 81.03% 81.30% +0.28%
- Complexity 2071 2110 +39
============================================
Files 211 215 +4
Lines 6593 6691 +98
Branches 782 785 +3
============================================
+ Hits 5342 5440 +98
+ Misses 951 949 -2
- Partials 300 302 +2
|
There was a problem hiding this comment.
Reviewed 29 of 29 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @Henning-Schulz)
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/agentstatus/AgentStatusManager.java, line 63 at r1 (raw file):
public void notifyAgentConfigurationFetched(Map<String, String> agentAttributes, Map<String, String> headers, AgentConfiguration resultConfiguration) { AgentMetaInformation metaInformation = AgentMetaInformation.of(headers); AgentHealth agentHealth = AgentHealth.valueOf(headers.getOrDefault(HEADER_AGENT_HEALTH, "OK"));
Should the default value be "OK"? Or would it be better to assume the status is not OK when this status is not explicitly set?
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/logging/logback/InternalProcessingAppender.java, line 29 at r1 (raw file):
public class InternalProcessingAppender extends AppenderBase<ILoggingEvent> { private static final NavigableMap<String, Class<?>> INVALIDATORS = new TreeMap<>();
I think we could use a short description here
Code quote:
private static final NavigableMap<inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/logging/logback/InternalProcessingAppender.java, line 33 at r1 (raw file):
private static final Map<Class<? extends Observer>, Observer> observers = new HashMap<>(); static {
Maybe a description would be nice here too
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/logging/logback/InternalProcessingAppender.java, line 102 at r1 (raw file):
* Observer to logging events. */ public interface Observer {
Maybe we should rename this to something like LoggingEventObserver since there is already an Observer Interface in java.until?
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/logging/logback/InternalProcessingAppender.java, line 110 at r1 (raw file):
* @param invalidator Class whose instances invalidate the log event when being sent as event, e.g., {@link rocks.inspectit.ocelot.core.config.InspectitConfigChangedEvent}. May be null. */ void onLoggingEvent(ILoggingEvent event, Class<?> invalidator);
In the classic Observer-pattern shouldn't there just be one update() function upon which the observer obtains the state from the observed object?
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/selfmonitoring/AgentHealthManager.java, line 42 at r1 (raw file):
private final SelfMonitoringService selfMonitoringService; private final Map<Class<?>, AgentHealth> invalidatableHealth = new ConcurrentHashMap<>();
I think we could use a short description here too.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/selfmonitoring/event/AgentHealthChangedEvent.java, line 18 at r1 (raw file):
*/ @Getter private final AgentHealth oldHealth;
Do we need this variable? Couldn't we also derive it from the AgentStatusManager?
In the Monitoring-Domain the default is "Unknown" if not clear, if it is OK or NOT OK. So it is clear, that the status is "Unknown". |
There was a problem hiding this comment.
Reviewable status: 22 of 29 files reviewed, 7 unresolved discussions (waiting on @MariusBrill)
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/agentstatus/AgentStatusManager.java, line 63 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Should the default value be "OK"? Or would it be better to assume the status is not OK when this status is not explicitly set?
Good point. If the status is not provided by the agent (e.g., because of an old version), let's do not assume one. I now set it only when provided, defaulting to null.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/logging/logback/InternalProcessingAppender.java, line 29 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
I think we could use a short description here
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/logging/logback/InternalProcessingAppender.java, line 33 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Maybe a description would be nice here too
As discussed, description above should be enough.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/logging/logback/InternalProcessingAppender.java, line 102 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Maybe we should rename this to something like LoggingEventObserver since there is already an Observer Interface in java.until?
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/logging/logback/InternalProcessingAppender.java, line 110 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
In the classic Observer-pattern shouldn't there just be one update() function upon which the observer obtains the state from the observed object?
You are right, this is not really an observer. I have renamed the class.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/selfmonitoring/AgentHealthManager.java, line 42 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
I think we could use a short description here too.
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/selfmonitoring/event/AgentHealthChangedEvent.java, line 18 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Do we need this variable? Couldn't we also derive it from the AgentStatusManager?
As discussed, we use the same pattern as in, e.g., InspectitConfigChangedEvent, which contains the old and new config.
There was a problem hiding this comment.
Reviewed 1 of 29 files at r1, 6 of 7 files at r2, all commit messages.
Reviewable status: 28 of 29 files reviewed, 9 unresolved discussions (waiting on @Henning-Schulz and @MariusBrill)
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/agentstatus/AgentStatusManager.java, line 63 at r1 (raw file):
Previously, Henning-Schulz (Henning Schulz) wrote…
Good point. If the status is not provided by the agent (e.g., because of an old version), let's do not assume one. I now set it only when provided, defaulting to null.
Based on the comment above, wouldn't it be more suitable to have an UNKNOWN default status?
inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/logging/logback/InternalProcessingAppenderTest.java, line 69 at r2 (raw file):
ILoggingEvent instrumentationEvent = new LoggingEvent("com.dummy.Method", (Logger) LoggerFactory.getLogger(InstrumentationManager.class), Level.INFO, "Dummy Info", new Throwable(), new String[]{}); InternalProcessingAppender.LogEventConsumer logEventConsumer = Mockito.mock(InternalProcessingAppender.LogEventConsumer.class);
you could also move this and the next line into the @BeforeEach method, or even create fields for the logEventConsumer and instrumentationEvent to avoid copy&paste.
The invocation count on the mock logEventConsumer could be reset in the @BeforeEach method
inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/selfmonitoring/AgentHealthManagerTest.java, line 55 at r2 (raw file):
executorService = new ScheduledThreadPoolExecutor(1); config = new InspectitConfig();
why do you set the config and the context in the @BeforeEach and not in a static @BeforeAll method?
There was a problem hiding this comment.
Reviewed 1 of 7 files at r2.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @Henning-Schulz and @MariusBrill)
There was a problem hiding this comment.
Reviewable status: 27 of 29 files reviewed, 7 unresolved discussions (waiting on @heiko-holz and @MariusBrill)
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/agentstatus/AgentStatusManager.java, line 63 at r1 (raw file):
Previously, heiko-holz (Heiko Holz) wrote…
Based on the comment above, wouldn't it be more suitable to have an
UNKNOWNdefault status?
That would mean we add UNKNOWN to the AgentHealth enum (which is used in the agent and the config server), which I honestly don't like. It does not make any sense in the agent, as it always knows is health. It only makes sense in the configuration server, in case an agent (because its version is too old) did not report a status. And it should be handled somehow special in that case. So I think it is better to have some special value that indicates no health is defined, a.k.a. null.
If we want to make the potential absence of a health value more explicit, we could use an Optional<AgentHealth>, but imho it does not really add any value here.
|
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/agentstatus/AgentStatusManager.java, line 63 at r1 (raw file): Previously, Henning-Schulz (Henning Schulz) wrote…
Okay, understood. I agree that |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @MariusBrill)
inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/logging/logback/InternalProcessingAppenderTest.java, line 24 at r3 (raw file):
public class InternalProcessingAppenderTest { private static final ILoggingEvent GENERAL_EVENT = new LoggingEvent("com.dummy.Method", (Logger) LoggerFactory.getLogger(LogMetricsRecorderTest.class), Level.INFO, "Dummy Info", new Throwable(), new String[]{});
Nice refactoring :)
There was a problem hiding this comment.
Reviewable status: 26 of 29 files reviewed, 7 unresolved discussions (waiting on @MariusBrill)
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/agentstatus/AgentStatusManager.java, line 63 at r1 (raw file):
Previously, heiko-holz (Heiko Holz) wrote…
Okay, understood.
I wasn't aware that it is only version-dependent. In a scenario where for example an error would prevent the agent of setting its health, theUNKNOWNvalue would be appropriate.I agree that
Optional<...>is not necessary.
Done.
Henning-Schulz
dismissed
MariusBrill’s stale review
Review finished by @heiko-holz
There was a problem hiding this comment.
Dismissed @MariusBrill from 7 discussions.
Reviewable status: 26 of 29 files reviewed, all discussions resolved (waiting on @MariusBrill)
There was a problem hiding this comment.
Reviewed 19 of 29 files at r1, 5 of 7 files at r2, 2 of 2 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @Henning-Schulz)
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:
This change is