-
Notifications
You must be signed in to change notification settings - Fork 205
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
HeartBeat Feature Implementation #631
Changes from 1 commit
bb3576b
8553cbd
1a0fa4e
edef438
aca4383
607b656
f147ff9
3430d26
efa1ff2
6bd5570
baba3b9
883fd72
912bae6
51f0258
704a11f
fd168aa
4c8d54e
6b8fe30
c31ff35
d844011
c8cff0e
63a6217
1e4ad3e
d33177c
757f0d1
7531583
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -161,9 +161,9 @@ public void setHeartBeatEnabled(boolean heartBeatEnabled) { | |
|
||
@Override | ||
public void initialize(TelemetryConfiguration configuration) { | ||
if (!isInitialized) { | ||
if (!isInitialized && isHeartBeatEnabled()) { | ||
synchronized (lock) { | ||
if (!isInitialized) { | ||
if (!isInitialized && isHeartBeatEnabled()) { | ||
this.heartBeatProviderInterface.initialize(configuration); | ||
InternalLogger.INSTANCE.info("heartbeat is enabled"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor: Heartbeat is initialized instead of enabled. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I renamed this variable in the latest comment. I agree it was misleading. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would go one step further: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great idea, I should probably add this check. It is usless to perform operation of initialization if the module is disabled. I think this should checked in conjucation with isHeartBeatEnabled. I will do the updates. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this change will work. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have already discussed this. This is addressed |
||
isInitialized = true; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
import com.microsoft.applicationinsights.TelemetryConfiguration; | ||
import com.microsoft.applicationinsights.internal.logger.InternalLogger; | ||
import com.microsoft.applicationinsights.internal.shutdown.Stoppable; | ||
import com.microsoft.applicationinsights.internal.util.ThreadPoolUtils; | ||
import com.microsoft.applicationinsights.telemetry.MetricTelemetry; | ||
import com.microsoft.applicationinsights.telemetry.Telemetry; | ||
import java.util.ArrayList; | ||
|
@@ -15,6 +16,7 @@ | |
import java.util.concurrent.Executors; | ||
import java.util.concurrent.ScheduledExecutorService; | ||
import java.util.concurrent.TimeUnit; | ||
import java.util.concurrent.atomic.AtomicInteger; | ||
import java.util.concurrent.atomic.AtomicLong; | ||
import org.apache.commons.lang3.StringUtils; | ||
import org.apache.commons.lang3.exception.ExceptionUtils; | ||
|
@@ -69,12 +71,12 @@ public class HeartBeatProvider implements HeartBeatProviderInterface, Stoppable | |
/** | ||
* ThreadPool used for adding properties to concurrent dictionary | ||
*/ | ||
private ExecutorService executorService = Executors.newCachedThreadPool(); | ||
private ExecutorService propertyUpdateService; | ||
|
||
/** | ||
* Threadpool used to send data heartbeat telemetry | ||
*/ | ||
private ScheduledExecutorService scheduledExecutorService = Executors.newScheduledThreadPool(1); | ||
private ScheduledExecutorService heartBeatSenderService; | ||
|
||
/** | ||
* Heartbeat enabled state | ||
|
@@ -88,6 +90,8 @@ public HeartBeatProvider() { | |
this.heartbeatProperties = new ConcurrentHashMap<>(); | ||
this.isEnabled = true; | ||
this.heartbeatsSent = new AtomicLong(0); | ||
this.propertyUpdateService = Executors.newCachedThreadPool(ThreadPoolUtils.createDaemonThreadFactory(HeartBeatProvider.class)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes these two thread pools indistinguishable. You may need another method in ThreadPool utils to specify a custom name. #Resolved |
||
this.heartBeatSenderService = Executors.newScheduledThreadPool(1, ThreadPoolUtils.createDaemonThreadFactory(HeartBeatProvider.class)); | ||
} | ||
|
||
@Override | ||
|
@@ -111,12 +115,10 @@ public void initialize(TelemetryConfiguration configuration) { | |
|
||
//Submit task to set properties to dictionary using separate thread. we do not wait for the | ||
//results to come out as some I/O bound properties may take time. | ||
executorService.submit(HeartbeatDefaultPayload.populateDefaultPayload(getExcludedHeartBeatProperties(), | ||
propertyUpdateService.submit(HeartbeatDefaultPayload.populateDefaultPayload(getExcludedHeartBeatProperties(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which properties are I/O bound? I wasn't able to find any. We may be able to remove the thread pool. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are no properties which are IO bound today. But later on there will be when we will query ARM to get VM ID. I would rather keep it. |
||
getExcludedHeartBeatPropertyProviders(), this)); | ||
|
||
|
||
|
||
scheduledExecutorService.scheduleAtFixedRate(heartBeatPulse(), interval, interval, TimeUnit.SECONDS); | ||
heartBeatSenderService.scheduleAtFixedRate(heartBeatPulse(), interval, interval, TimeUnit.SECONDS); | ||
} | ||
} | ||
|
||
|
@@ -133,10 +135,10 @@ public boolean addHeartBeatProperty(String propertyName, String propertyValue, | |
payload.setPayloadValue(propertyValue); | ||
heartbeatProperties.put(propertyName, payload); | ||
isAdded = true; | ||
InternalLogger.INSTANCE.trace("added heartbeat property"); | ||
InternalLogger.INSTANCE.trace("added heartbeat property %s - %s", propertyName, propertyValue); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hyphen is an interesting choice. Usually you see There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is nit: I think should be fine :) |
||
} | ||
else { | ||
throw new Exception("heartbeat property cannot be added twice"); | ||
throw new Exception("heartbeat property cannot be added twice. Please use setHeartBeatProperty instead to modify the value"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use a more specific Exception subclass. #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is nice to have but I don't think this should block the PR. #Resolved |
||
} | ||
} catch (Exception e) { | ||
InternalLogger.INSTANCE.warn("Failed to add the property %s value %s, stack trace is : %s," , | ||
|
@@ -156,17 +158,21 @@ public boolean setHeartBeatProperty(String propertyName, String propertyValue, | |
boolean setResult = false; | ||
if (!StringUtils.isEmpty(propertyName)) { | ||
try { | ||
if (heartbeatProperties.containsKey(propertyName) && !HeartbeatDefaultPayload.isDefaultKeyWord(propertyName)) { | ||
|
||
if (!heartbeatProperties.containsKey(propertyName)) { | ||
setResult = false; | ||
throw new Exception("heartbeat property cannot be set without adding it first"); | ||
} else if (HeartbeatDefaultPayload.isDefaultKeyword(propertyName)) { | ||
setResult = false; | ||
throw new Exception("heartbeat beat property specified is a reserved property"); | ||
} | ||
else { | ||
HeartBeatPropertyPayload payload = new HeartBeatPropertyPayload(); | ||
payload.setHealthy(isHealthy); | ||
payload.setPayloadValue(propertyValue); | ||
heartbeatProperties.put(propertyName, payload); | ||
setResult = true; | ||
} | ||
else { | ||
throw new Exception("heartbeat property cannot be set without adding it first, or heartbeat" | ||
+ "beat property specified is a reserved property"); | ||
} | ||
} | ||
catch (Exception e) { | ||
InternalLogger.INSTANCE.warn("failed to set heartbeat property name %s, value %s, " | ||
|
@@ -228,11 +234,11 @@ public void setExcludedHeartBeatProperties(List<String> excludedHeartBeatPropert | |
|
||
@Override | ||
public void stop(long timeout, TimeUnit timeUnit) { | ||
executorService.shutdown(); | ||
scheduledExecutorService.shutdown(); | ||
propertyUpdateService.shutdown(); | ||
heartBeatSenderService.shutdown(); | ||
try { | ||
executorService.awaitTermination(timeout, timeUnit); | ||
scheduledExecutorService.awaitTermination(timeout, timeUnit); | ||
propertyUpdateService.awaitTermination(timeout, timeUnit); | ||
heartBeatSenderService.awaitTermination(timeout, timeUnit); | ||
} | ||
catch (InterruptedException e) { | ||
InternalLogger.INSTANCE.warn("unable to successfully terminate heartbeat module, " | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,24 +6,24 @@ | |
|
||
public class HeartbeatDefaultPayload { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would think this class is the Payload, but really its delegating things to PayloadProviders. Consider renaming. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is infact a container of default heartbeat providers and I think should be fine. |
||
|
||
private static final List<HeartBeatDefaultPayloadProviderInterface> defaultPayloadProviders = | ||
private static final List<HeartBeatPayloadProviderInterface> defaultPayloadProviders = | ||
new ArrayList<>(); | ||
|
||
static { | ||
defaultPayloadProviders.add(new BaseDefaultHeartbeatPropertyProvider()); | ||
defaultPayloadProviders.add(new WebAppsDefaultHeartbeatProvider()); | ||
defaultPayloadProviders.add(new DefaultHeartbeatPropertyProvider()); | ||
defaultPayloadProviders.add(new WebAppsHeartbeatProvider()); | ||
} | ||
|
||
public static boolean isDefaultKeyWord(String keyword) { | ||
for (HeartBeatDefaultPayloadProviderInterface payloadProvider : defaultPayloadProviders) { | ||
if (payloadProvider.isKeyWord(keyword)) { | ||
public static boolean isDefaultKeyword(String keyword) { | ||
for (HeartBeatPayloadProviderInterface payloadProvider : defaultPayloadProviders) { | ||
if (payloadProvider.isKeyword(keyword)) { | ||
return true; | ||
} | ||
} | ||
return false; | ||
} | ||
|
||
public static boolean addDefaultPayLoadProvider(HeartBeatDefaultPayloadProviderInterface payloadProviderInterface) { | ||
public static boolean addDefaultPayLoadProvider(HeartBeatPayloadProviderInterface payloadProviderInterface) { | ||
if (payloadProviderInterface != null) { | ||
defaultPayloadProviders.add(payloadProviderInterface); | ||
return true; | ||
|
@@ -38,7 +38,7 @@ public static Callable<Boolean> populateDefaultPayload(final List<String> disabl | |
volatile boolean populatedFields = false; | ||
@Override | ||
public Boolean call() throws Exception { | ||
for (HeartBeatDefaultPayloadProviderInterface payloadProvider : defaultPayloadProviders) { | ||
for (HeartBeatPayloadProviderInterface payloadProvider : defaultPayloadProviders) { | ||
if (disabledProviders != null && disabledProviders.contains(payloadProvider.getName())) { | ||
|
||
// skip any azure specific modules here | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want the OS version? If so, you should explore the difference between
os.name
andos.version
(i.e.os.name
does not contain version information).I also found a few articles stating that
os.name
doesn't have the correct value when on Windows 10. Perhaps there is a different way to find this information.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can amend it, if there is a better way though I am not aware of one. This is meta data which is not very very vital though.