Skip to content

Commit

Permalink
Merge pull request #7160 from gucalder/preview
Browse files Browse the repository at this point in the history 
[Insights] Fixing issues #6833 and #7102: failure during creation if categories are included
  • Loading branch information
@cormacpayne
cormacpayne authored Sep 11, 2018
2 parents 1afb1b0 + 45077aa commit fd28f1f
Show file tree
Hide file tree
Showing 3 changed files with 88 additions and 18 deletions.
27 changes: 18 additions & 9 deletions ...ceManager/Insights/Commands.Insights.Test/Diagnostics/SetDiagnosticSettingCommandTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,14 +203,23 @@ public void SetSomeCategories()
expectedSettings.Logs[0].Enabled = false;

VerifyCalledOnce();
VerifySettings(expectedSettings, this.calledSettings);
VerifySettings(expectedSettings, this.calledSettings, suffix: "#1");

// Testing the new categories must be known before the cmdlet can add them
expectedSettings.Logs.Add(
new LogSettings()
{
Category = "TestCategory3",
RetentionPolicy = new RetentionPolicy
{
Days = 0,
Enabled = false
}
});
cmdlet.Categories = new List<string> { "TestCategory3" };
cmdlet.Enabled = false;
cmdlet.MyInvocation.BoundParameters[SetAzureRmDiagnosticSettingCommand.EnabledParamName] = false;
var argException = Assert.Throws<PSInvalidOperationException>(() => cmdlet.ExecuteCmdlet());
Assert.Contains("ArgumentException", argException.ToString());
cmdlet.ExecuteCmdlet();

// Testing the new metric categories must be known before the cmdlet can add them
expectedSettings.Metrics[0].Enabled = false;
Expand All @@ -220,14 +229,13 @@ public void SetSomeCategories()
cmdlet.MyInvocation.BoundParameters[SetAzureRmDiagnosticSettingCommand.EnabledParamName] = false;
cmdlet.ExecuteCmdlet();

VerifySettings(expectedSettings, this.calledSettings);
VerifySettings(expectedSettings, this.calledSettings, suffix: "#2");

// Testing the new categories must be known before the cmdlet can add them
cmdlet.MetricCategory = new List<string> { "MetricCat3" };
cmdlet.Enabled = false;
cmdlet.MyInvocation.BoundParameters[SetAzureRmDiagnosticSettingCommand.EnabledParamName] = false;
argException = Assert.Throws<PSInvalidOperationException>(() => cmdlet.ExecuteCmdlet());
Assert.Contains("ArgumentException", argException.ToString());
cmdlet.ExecuteCmdlet();
}

[Fact]
Expand Down Expand Up @@ -310,7 +318,8 @@ private void VerifyCalledOnce()

private void VerifySettings(
DiagnosticSettingsResource expectedSettings,
DiagnosticSettingsResource actualSettings)
DiagnosticSettingsResource actualSettings,
string suffix = "")
{
Assert.Equal(expectedSettings.StorageAccountId, actualSettings.StorageAccountId);
Assert.Equal(expectedSettings.EventHubName, actualSettings.EventHubName);
Expand All @@ -321,7 +330,7 @@ private void VerifySettings(
}
else
{
Assert.True(expectedSettings.Logs.Count == actualSettings.Logs.Count, string.Format("Expected: {0}, Actual: {1}, no the same number of Log settings", expectedSettings.Logs.Count, actualSettings.Logs.Count));
Assert.True(expectedSettings.Logs.Count == actualSettings.Logs.Count, string.Format("Expected: {0}, Actual: {1}, no the same number of Log settings {2}", expectedSettings.Logs.Count, actualSettings.Logs.Count, suffix));
for (int i = 0; i < expectedSettings.Logs.Count; i++)
{
var expected = expectedSettings.Logs[i];
Expand All @@ -338,7 +347,7 @@ private void VerifySettings(
}
else
{
Assert.True(expectedSettings.Metrics.Count == actualSettings.Metrics.Count, string.Format("Expected: {0}, Actual: {1}, no the same number of Metric settings", expectedSettings.Metrics.Count, actualSettings.Metrics.Count));
Assert.True(expectedSettings.Metrics.Count == actualSettings.Metrics.Count, string.Format("Expected: {0}, Actual: {1}, no the same number of Metric settings {2}", expectedSettings.Metrics.Count, actualSettings.Metrics.Count, suffix));
for (int i = 0; i < expectedSettings.Metrics.Count; i++)
{
var expected = expectedSettings.Metrics[i];
Expand Down
7 changes: 7 additions & 0 deletions src/ResourceManager/Insights/Commands.Insights/ChangeLog.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@
-->
## Current Release

* Fixed issues #6833 and #7102 (Diagnostic Settings area)
- Issues with the default name, i.e. "service", during creation and listing/getting of diagnostic settings
- Issues creating diagnostic settings with categories

* Added deprecation message for metrics time grains parameters
- Timegrains parameters are still being accepted (this is a non-breaking change,) but they are ignored in the backend since only PT1M is valid

## Version 5.1.3
* Fixed issue with default resource groups not being set.
* Updated common runtime assemblies
Expand Down
72 changes: 63 additions & 9 deletions ...ourceManager/Insights/Commands.Insights/Diagnostics/SetAzureRmDiagnosticSettingCommand.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -364,8 +364,13 @@ private void SetSelectedTimegrains(DiagnosticSettingsResource properties)
throw new ArgumentException("Parameter 'Enabled' is required by 'Timegrains' parameter.");
}

if (this.Timegrains.Count > 0)
if (this.Timegrains != null && this.Timegrains.Count > 0)
{
if (properties.Metrics == null)
{
properties.Metrics = new List<MetricSettings>();
}

WriteWarningWithTimestamp("Deprecation: The timegain argument for metrics will be deprecated since the back end supports only PT1M. Currently it is ignored for backwards compatibility.");
WriteDebugWithTimestamp("Setting Enabled property for metrics since timegrains argument is non-empty");
foreach (MetricSettings metric in properties.Metrics)
Expand All @@ -383,16 +388,35 @@ private void SetSelectedCategories(DiagnosticSettingsResource properties)
}

WriteDebugWithTimestamp("Setting log categories, including Enabled property");
if (properties.Logs == null)
{
properties.Logs = new List<LogSettings>();
}

foreach (string category in this.Categories)
{
LogSettings logSettings = properties.Logs.FirstOrDefault(x => string.Equals(x.Category, category, StringComparison.OrdinalIgnoreCase));

if (logSettings == null)
{
throw new ArgumentException(string.Format(CultureInfo.InvariantCulture, "Log category '{0}' is not available", category));
}
// if not there add it
logSettings = new LogSettings()
{
Category = category,
RetentionPolicy = new RetentionPolicy
{
Days = 0,
Enabled = false
},
Enabled = this.Enabled
};

logSettings.Enabled = this.Enabled;
properties.Logs.Add(logSettings);
}
else
{
// else update it
logSettings.Enabled = this.Enabled;
}
}
}

Expand All @@ -404,16 +428,37 @@ private void SetSelectedMetricsCategories(DiagnosticSettingsResource properties)
}

WriteDebugWithTimestamp("Setting metric categories, including Enabled property");
if (properties.Metrics == null)
{
properties.Metrics = new List<MetricSettings>();
}

foreach (string category in this.MetricCategory)
{
MetricSettings metricSettings = properties.Metrics.FirstOrDefault(x => string.Equals(x.Category, category, StringComparison.OrdinalIgnoreCase));

if (metricSettings == null)
{
throw new ArgumentException(string.Format(CultureInfo.InvariantCulture, "Metric category '{0}' is not available", category));
}
// If not there add it
metricSettings = new MetricSettings
{
Category = category,
Enabled = this.Enabled,
RetentionPolicy = new RetentionPolicy
{
Days = 0,
Enabled = false
},
TimeGrain = null
};

metricSettings.Enabled = this.Enabled;
properties.Metrics.Add(metricSettings);
}
else
{
// else update it
metricSettings.Enabled = this.Enabled;
}
}
}

Expand All @@ -425,12 +470,22 @@ private void SetAllCategoriesAndTimegrains(DiagnosticSettingsResource properties
}

WriteDebugWithTimestamp("Setting Enabled property for logs");
if (properties.Logs == null)
{
properties.Logs = new List<LogSettings>();
}

foreach (var log in properties.Logs)
{
log.Enabled = this.Enabled;
}

WriteDebugWithTimestamp("Setting Enabled property for metrics");
if (properties.Metrics == null)
{
properties.Metrics = new List<MetricSettings>();
}

foreach (var metric in properties.Metrics)
{
metric.Enabled = this.Enabled;
Expand Down Expand Up @@ -470,7 +525,6 @@ private void SetEventHubRule(DiagnosticSettingsResource properties)
}
}


private void SetStorage(DiagnosticSettingsResource properties)
{
if (this.isStorageParamPresent)
Expand Down

0 comments on commit fd28f1f

Please sign in to comment.