Conversation
rewrote to Azure Bindings Changed filter strategy
rewrote to Azure Bindings Changed filter strategy
adityajagtiani89
left a comment
adityajagtiani89
left a comment
There was a problem hiding this comment.
Hi Michael, I've added some comments below. Please feel free to reach out to me for clarification on any of these points.
| implementation="org.apache.maven.plugins.shade.resource.ServicesResourceTransformer"/> | ||
| <transformer | ||
| implementation="org.apache.maven.plugins.shade.resource.ManifestResourceTransformer"> | ||
| <transformer implementation="org.apache.maven.plugins.shade.resource.ServicesResourceTransformer"/> |
There was a problem hiding this comment.
Please make sure you're shipping a LICENSE.txt and NOTICE.txt along with the existing artifacts. You can find several examples of how this is done in the pom.xml from our recent extensions (Netscaler is a good reference)
|
|
||
| @Override | ||
| public String getMonitorName() { | ||
| return "Azure Monitoring Extension"; |
There was a problem hiding this comment.
"Azure Monitoring Extension" can also go to the Constants class, just like the Default Metric Prefix above.
| else { logger.error("The config.yml is not loaded due to previous errors.The task will not run"); } | ||
| @Override | ||
| protected void doRun(TasksExecutionServiceProvider tasksExecutionServiceProvider) { | ||
| List<Map<String,?>> subscriptions = (List<Map<String,?>>)getContextConfiguration().getConfigYml().get("subscriptions"); |
There was a problem hiding this comment.
A cleaner approach would be to create a global called configYml, for example, and fetching it from the MonitorContextConfiguration once. I see you've done it several times. You can then use this configYml object to get your subscriptions, serviceFabrics etc.
| private final MetricWriteHelper metricWriteHelper; | ||
| private final Map<String, ?> subscription; | ||
| private final List<Map<String,?>> resourceGroupFilters; | ||
| private final CountDownLatch countDownLatch; |
There was a problem hiding this comment.
I'm a little unclear as to why this CountDownLatch is required. I see you've created another one on line 65 that covers your Azure resources. What is the benefit of having this additional one?
| String currentResourceTypeFilter = resourceTypeFilter.get("resourceType").toString(); | ||
| AzureMetrics azureMetricsTask = new AzureMetrics( | ||
| resourceFilter, | ||
| currentResourceGroupFilter, |
There was a problem hiding this comment.
This is a very expensive operation (n ^ 4). I understand why it is that way, looking at the current hierarchy in your config.yml but I'm wondering if this can be done more efficiently. Any thoughts?
| logger.debug("Metric List is empty"); | ||
| } | ||
| } else { | ||
| metricWriteHelper.transformAndPrintMetrics(finalMetricList); |
There was a problem hiding this comment.
We have started publishing a 'Heart Beat' metric per configured instance. It's a 0/1 metric that prints 0 in case of an error/connectivity issues etc and 1 in case of successful metric collection. You can refer to the Netscaler Extension for how this is done.
| public class Constants { | ||
| public static final String DEFAULT_METRIC_PREFIX = "Custom Metrics|AzureMonitor|"; | ||
| public static final String METRIC_SEPARATOR = "|"; | ||
| public static Azure.Authenticated azureMonitorAuth = null; |
| * The copyright notice above does not evidence any actual or intended publication of such source code. | ||
| * | ||
| */ | ||
|
|
There was a problem hiding this comment.
This test class is not needed at all. It does not test anything whatsoever.
| */ | ||
|
|
||
| package com.appdynamics.monitors.azure; | ||
|
|
There was a problem hiding this comment.
There should be comprehensive unit tests for your AzureMetricsProcessor, as that's where most of the heavy lifting is happening. You'll probably have to mock a connection to the Azure API and create some dummy data in your test resources. Again, the Netscaler extension is a good reference point.
| try { | ||
| url = new URL(input); | ||
| } catch (MalformedURLException e) { | ||
| logger.error("Error forming our from String {}", input, e); |
There was a problem hiding this comment.
"Error forming our URL from String {}".
…be sent when using dimensions as well
3 participants
No description provided.