-
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 17 commits
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 |
---|---|---|
|
@@ -21,6 +21,7 @@ | |
|
||
package com.microsoft.applicationinsights.internal.config; | ||
|
||
import com.microsoft.applicationinsights.internal.heartbeat.HeartBeatModule; | ||
import java.io.InputStream; | ||
import java.util.Map; | ||
import java.util.List; | ||
|
@@ -119,7 +120,9 @@ public final void initialize(TelemetryConfiguration configuration) { | |
private void setMinimumConfiguration(ApplicationInsightsXmlConfiguration userConfiguration, TelemetryConfiguration configuration) { | ||
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. Will the heartbeat module always be added? It didn't look like it the way this method is called. #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. Yes this module will always be added. #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. @littleaj look into the method 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. @dhaval24 , Ok, I see. I did notice that you can reduce some redundancy if you remove the call to 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. @littleaj I need to add this for SpringBoot scenarios. Here it is not called because SpringBoot autoconfigures it using beans. #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. |
||
setInstrumentationKey(userConfiguration, configuration); | ||
configuration.setChannel(new InProcessTelemetryChannel()); | ||
addHeartBeatModule(configuration); | ||
setContextInitializers(null, configuration); | ||
initializeComponents(configuration); | ||
} | ||
|
||
private void setInternalLogger(SDKLoggerXmlElement sdkLogger, TelemetryConfiguration configuration) { | ||
|
@@ -173,7 +176,13 @@ private void setTelemetryModules(ApplicationInsightsXmlConfiguration appConfigur | |
ReflectionUtils.loadComponents(TelemetryModule.class, modules, configurationModules.getAdds()); | ||
} | ||
|
||
//if heartbeat module is not loaded, load heartbeat module | ||
if (!isHeartBeatModuleAdded(modules)) { | ||
addHeartBeatModule(configuration); | ||
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. Is this behavior common across AI SDKs implementing heartbeat? I would not want customers remember in which SDK they would need to add HeartBeat explicitly if replacing modules vs. where it will be added by default even if they specifically didn't mention it. 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. @Dmitry-Matveev in .net SDK DiagnosticModule is used to enable HeartBeat module and I believe DiagnosticModule is turned on by default. However, in Java SDK there is no diagnostic module and if we want to enable heartbeat module by default if not specified in the configuration file I think this would be the way. If you have better option I would like to hear that. |
||
} | ||
|
||
List<TelemetryModule> pcModules = getPerformanceModules(appConfiguration.getPerformance()); | ||
|
||
modules.addAll(pcModules); | ||
} | ||
|
||
|
@@ -502,6 +511,27 @@ private void initializeComponents(TelemetryConfiguration configuration) { | |
} | ||
} | ||
|
||
/** | ||
* Adds heartbeat module with default configuration | ||
* @param configuration TelemetryConfiguration Instance | ||
*/ | ||
private void addHeartBeatModule(TelemetryConfiguration configuration) { | ||
HeartBeatModule module = new HeartBeatModule(new HashMap<String, String>()); | ||
configuration.getTelemetryModules().add(module); | ||
} | ||
|
||
/** | ||
* Checks if heartbeat module is present | ||
* @param module List of modules in current TelemetryConfiguration Instance | ||
* @return true if heartbeat module is present | ||
*/ | ||
private boolean isHeartBeatModuleAdded(List<TelemetryModule> module) { | ||
for (TelemetryModule mod : module) { | ||
if (mod instanceof HeartBeatModule) return true; | ||
} | ||
return false; | ||
} | ||
|
||
void setPerformanceCountersSection(String performanceCountersSection) { | ||
this.performanceCountersSection = performanceCountersSection; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,162 @@ | ||
package com.microsoft.applicationinsights.internal.heartbeat; | ||
|
||
import com.microsoft.applicationinsights.internal.logger.InternalLogger; | ||
import com.microsoft.applicationinsights.internal.util.PropertyHelper; | ||
import java.util.HashSet; | ||
import java.util.List; | ||
import java.util.Properties; | ||
import java.util.Set; | ||
import java.util.UUID; | ||
import java.util.concurrent.Callable; | ||
import org.apache.commons.lang3.exception.ExceptionUtils; | ||
|
||
/** | ||
* <h1>Base Heartbeat Property Provider</h1> | ||
* | ||
* <p> | ||
* This class is a concrete implementation of {@link com.microsoft.applicationinsights.internal.heartbeat.HeartBeatDefaultPayloadProviderInterface} | ||
* It enables setting SDK Metadata to heartbeat payload. | ||
* </p> | ||
* | ||
* @author Dhaval Doshi | ||
* @since 03-30-2018 | ||
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 remove the The same for the other 7 occurrences of |
||
*/ | ||
public class BaseDefaultHeartbeatPropertyProvider implements HeartBeatDefaultPayloadProviderInterface { | ||
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'd suggest renaming this class to 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. Fixed |
||
|
||
/** | ||
* Collection holding default properties for this default provider. | ||
*/ | ||
private final Set<String> defaultFields; | ||
|
||
/** | ||
* Random GUID that would help in analysis when app has stopped and restarted. Each restart will | ||
* have a new GUID. If the application is unstable and goes through frequent restarts this will help | ||
* us identify instability in the analytics backend. | ||
*/ | ||
private static UUID uniqueProcessId; | ||
|
||
/** | ||
* Name of this provider. | ||
*/ | ||
private final String name = "Base"; | ||
|
||
public BaseDefaultHeartbeatPropertyProvider() { | ||
defaultFields = new HashSet<>(); | ||
initializeDefaultFields(defaultFields); | ||
} | ||
|
||
@Override | ||
public String getName() { | ||
return this.name; | ||
} | ||
|
||
@Override | ||
public boolean isKeyWord(String keyword) { | ||
return defaultFields.contains(keyword); | ||
} | ||
|
||
@Override | ||
public Callable<Boolean> setDefaultPayload(final List<String> disableFields, | ||
final HeartBeatProviderInterface provider) { | ||
return new Callable<Boolean>() { | ||
|
||
// using volatile here to avoid caching in threads. | ||
volatile boolean hasSetValues = false; | ||
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 doesn't need to be volatile. You're reasoning in the comment is incorrect. No other thread can change this value. In fact, these should just be local to the #Resolved |
||
volatile Set<String> enabledProperties = MiscUtils.except(defaultFields, disableFields); | ||
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 doesn't need to be volatile. The reference is not updated. 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. Fixed |
||
@Override | ||
public Boolean call() { | ||
for (String fieldName : enabledProperties) { | ||
try { | ||
switch (fieldName) { | ||
case "jdkVersion": | ||
provider.addHeartBeatProperty(fieldName, getJdkVersion(), true); | ||
hasSetValues = true; | ||
break; | ||
case "sdk-version": | ||
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. Normally, I would be consistent with the naming scheme (hyphenated-case vs. camelCase). Is this in the HB event spec? #ByDesign 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. @littleaj I agree with your point. I still need to match the naming scheme of the .NET. So there can be something off in the names here. #ByDesign 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. makes sense #ByDesign |
||
provider.addHeartBeatProperty(fieldName, getSdkVersion(), true); | ||
hasSetValues = true; | ||
break; | ||
case "osType": | ||
provider.addHeartBeatProperty(fieldName, getOsType(), true); | ||
hasSetValues = true; | ||
break; | ||
case "processSessionId": | ||
provider.addHeartBeatProperty(fieldName, getProcessSessionId(), true); | ||
hasSetValues = true; | ||
break; | ||
default: | ||
//We won't accept unknown properties in default providers. | ||
InternalLogger.INSTANCE.trace("Encountered unknown default property"); | ||
break; | ||
} | ||
} | ||
catch (Exception e) { | ||
InternalLogger.INSTANCE.warn("Failed to obtain heartbeat property, stack trace" | ||
+ "is: %s", ExceptionUtils.getStackTrace(e)); | ||
} | ||
} | ||
return hasSetValues; | ||
} | ||
}; | ||
} | ||
|
||
/** | ||
* This method initializes the collection with Default Properties of this provider. | ||
* @param defaultFields collection to hold default properties. | ||
*/ | ||
private void initializeDefaultFields(Set<String> defaultFields) { | ||
|
||
if (defaultFields == null) { | ||
defaultFields = new HashSet<>(); | ||
} | ||
defaultFields.add("jdkVersion"); | ||
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. better if all these constants are moved to a single place, declared as constants. 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. @cijothomas Might be a good idea, but since there are different classes for each Different kind of providers I think doesn't add a lot of value since the separation of concerns is already addressed. 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's generally good practice to refactor these as constants; especially if you have string "keys" that are consistent (especially if you use them in tests). 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. @littleaj this is addressed 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. Same comment as below: jreVersion 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. Fixed |
||
defaultFields.add("sdk-version"); | ||
defaultFields.add("osType"); | ||
defaultFields.add("processSessionId"); | ||
} | ||
|
||
/** | ||
* Gets the JDK version being used by the application | ||
* @return String representing JDK Version | ||
*/ | ||
private String getJdkVersion() { | ||
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's not the JdkVersion it's the JreVersion 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. Fixed |
||
return System.getProperty("java.version"); | ||
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. Is this the version on Java runtime on which the application is running? (Is there distinction between 'application is compiled against 1.6 version, but running in java runtime 1.8) 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 tells the JRE on which application is running. No there is no distinction as such. So in your above example it would report 1.8 |
||
} | ||
|
||
/** | ||
* Returns the Application Insights SDK version user is using to instrument his application | ||
* @return returns string prefixed with "java" representing the Application Insights Java | ||
* SDK version. | ||
*/ | ||
private String getSdkVersion() { | ||
|
||
String sdkVersion = "java"; | ||
|
||
Properties sdkVersionProps = PropertyHelper.getSdkVersionProperties(); | ||
if (sdkVersionProps != null) { | ||
String version = sdkVersionProps.getProperty("version"); | ||
sdkVersion = String.format("java:%s", version); | ||
return sdkVersion; | ||
} | ||
return sdkVersion + ":unknown-version"; | ||
} | ||
|
||
/** | ||
* Gets the OS version on which application is running. | ||
* @return String representing OS version | ||
*/ | ||
private String getOsType() { | ||
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. From the name OsType, I expect it to simply return Windows/Mac/Linux/WindowsServer only, not the full version. like Windows10.4.89. 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. Makes sense. I can do this. 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. Addressed |
||
return System.getProperty("os.name"); | ||
} | ||
|
||
/** | ||
* Returns the Unique GUID for the application's current session. | ||
* @return String representing GUID for each running session | ||
*/ | ||
private String getProcessSessionId() { | ||
if (uniqueProcessId == null) { | ||
uniqueProcessId = UUID.randomUUID(); | ||
} | ||
return uniqueProcessId.toString(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
package com.microsoft.applicationinsights.internal.heartbeat; | ||
|
||
import java.util.List; | ||
import java.util.concurrent.Callable; | ||
|
||
/** | ||
* <h1>Interface for setting default properties</h1> | ||
* | ||
* <p> | ||
* This interface is used to set the default payload of a provider and defines implementation for | ||
* some helper methods to assist it. | ||
* | ||
* The default concrete implementations are {@link com.microsoft.applicationinsights.internal.heartbeat.BaseDefaultHeartbeatPropertyProvider} | ||
* and {@link com.microsoft.applicationinsights.internal.heartbeat.WebAppsDefaultHeartbeatProvider} | ||
* </p> | ||
* | ||
* @author Dhaval Doshi | ||
* @since 03-30-2018 | ||
*/ | ||
public interface HeartBeatDefaultPayloadProviderInterface { | ||
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 applies to all uses of 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 think it should be |
||
|
||
/** | ||
* Returns the name of the heartbeat provider. | ||
* @return Name of the heartbeat provider | ||
*/ | ||
String getName(); | ||
|
||
/** | ||
* Tells if the input string is a reserved property. | ||
* @param keyword string to test | ||
* @return true if input string is reserved keyword | ||
*/ | ||
boolean isKeyWord(String keyword); | ||
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. Same here. 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. Fixed |
||
|
||
/** | ||
* Returns a callable which can be executed to set the payload based on the parameters. | ||
* @param disableFields List of Properties to be excluded from payload | ||
* @param provider The current heartbeat provider | ||
* @return Callable which can be executed to add the payload | ||
*/ | ||
Callable<Boolean> setDefaultPayload(List<String> disableFields, HeartBeatProviderInterface provider); | ||
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. Nitpick: I would consider renaming this method since getXyz and setXyz are generally recognized as field accessors. 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. @littleaj this was my thought, but I wanted to keep API very similar to C# implementation in this case following java camel case for consistency. Let me know if you still think that we should rename. |
||
|
||
} |
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.
rephrase to something like 'Heartbeat feature which sends periodic hearbeats with basic info about the app/runtime to Application Insights'
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.
I think this sentence is sufficient. I can link the PR for description.
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.
I agree with @cijothomas. Since Heartbeat hasn't existed in the past, saying "Heartbeat support" is odd to me. I would also avoid the phrase "monitor application health" since "application health" is domain specific and that's not what the heartbeat feature is about. #Closed
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.
Done