-
Notifications
You must be signed in to change notification settings - Fork 50
Change ASB usage data collection method to prevent elevated privileges #5535
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
Open
mikeminutillo
wants to merge
1
commit into
collect-more-data-cloud
Choose a base branch
from
change-asb-metrics-package
base: collect-more-data-cloud
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+26
−85
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,9 +12,9 @@ namespace ServiceControl.Transports.ASBS; | |
| using Azure.Core; | ||
| using Azure.Core.Pipeline; | ||
| using Azure.Identity; | ||
| using Azure.Monitor.Query.Metrics; | ||
| using Azure.Monitor.Query.Metrics.Models; | ||
| using Azure.ResourceManager; | ||
| using Azure.ResourceManager.Monitor; | ||
| using Azure.ResourceManager.Monitor.Models; | ||
| using Azure.ResourceManager.Resources; | ||
| using Azure.ResourceManager.ServiceBus; | ||
| using BrokerThroughput; | ||
|
|
@@ -34,8 +34,6 @@ public class AzureQuery(ILogger<AzureQuery> logger, TimeProvider timeProvider, T | |
| TokenCredential? credential; | ||
| ResourceIdentifier? resourceId; | ||
| ArmEnvironment armEnvironment; | ||
| MetricsClientAudience metricsClientAudience; | ||
| MetricsClient? metricsClient; | ||
|
|
||
| protected override void InitializeCore(ReadOnlyDictionary<string, string> settings) | ||
| { | ||
|
|
@@ -108,7 +106,7 @@ protected override void InitializeCore(ReadOnlyDictionary<string, string> settin | |
| Diagnostics.AppendLine("Client secret set"); | ||
| } | ||
|
|
||
| (armEnvironment, metricsClientAudience) = GetEnvironment(); | ||
| armEnvironment = GetEnvironment(); | ||
|
|
||
| if (managementUrl == null) | ||
| { | ||
|
|
@@ -147,26 +145,26 @@ protected override void InitializeCore(ReadOnlyDictionary<string, string> settin | |
|
|
||
| return; | ||
|
|
||
| (ArmEnvironment armEnvironment, MetricsClientAudience metricsClientAudience) GetEnvironment() | ||
| ArmEnvironment GetEnvironment() | ||
| { | ||
| if (managementUrlParsed == null || managementUrlParsed == ArmEnvironment.AzurePublicCloud.Endpoint) | ||
| { | ||
| return (ArmEnvironment.AzurePublicCloud, MetricsClientAudience.AzurePublicCloud); | ||
| return ArmEnvironment.AzurePublicCloud; | ||
| } | ||
|
|
||
| if (managementUrlParsed == ArmEnvironment.AzureChina.Endpoint) | ||
| { | ||
| return (ArmEnvironment.AzureChina, MetricsClientAudience.AzureChina); | ||
| return ArmEnvironment.AzureChina; | ||
| } | ||
|
|
||
| if (managementUrlParsed == ArmEnvironment.AzureGermany.Endpoint) | ||
| { | ||
| return (ArmEnvironment.AzureGermany, MetricsClientAudience.AzurePublicCloud); | ||
| return ArmEnvironment.AzureGermany; | ||
| } | ||
|
|
||
| if (managementUrlParsed == ArmEnvironment.AzureGovernment.Endpoint) | ||
| { | ||
| return (ArmEnvironment.AzureGovernment, MetricsClientAudience.AzureGovernment); | ||
| return ArmEnvironment.AzureGovernment; | ||
| } | ||
|
|
||
| string options = string.Join(", ", | ||
|
|
@@ -176,7 +174,7 @@ protected override void InitializeCore(ReadOnlyDictionary<string, string> settin | |
| }.Select(environment => $"\"{environment.Endpoint}\"")); | ||
| InitialiseErrors.Add($"Management url configuration is invalid, available options are {options}"); | ||
|
|
||
| return (ArmEnvironment.AzurePublicCloud, MetricsClientAudience.AzurePublicCloud); | ||
| return ArmEnvironment.AzurePublicCloud; | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -241,85 +239,28 @@ public override async IAsyncEnumerable<QueueThroughput> GetThroughputPerDay(IBro | |
| } | ||
| } | ||
|
|
||
| async Task<MetricsClient> InitializeMetricsClient(CancellationToken cancellationToken = default) | ||
| { | ||
| if (resourceId is null || armClient is null || credential is null) | ||
| { | ||
| throw new InvalidOperationException("AzureQuery has not been initialized correctly."); | ||
| } | ||
|
|
||
| var serviceBusNamespaceResource = await armClient | ||
| .GetServiceBusNamespaceResource(resourceId).GetAsync(cancellationToken) | ||
| ?? throw new Exception($"Could not find an Azure Service Bus namespace with resource Id: \"{resourceId}\""); | ||
|
|
||
| // Determine the region of the namespace | ||
| var regionName = serviceBusNamespaceResource.Value.Data.Location.Name; | ||
|
|
||
| // Build the regional Azure Monitor Metrics endpoint from the audience | ||
| var metricsEndpoint = BuildMetricsEndpoint(metricsClientAudience, regionName); | ||
|
|
||
| // CreateNewOnMetadataUpdateAttribute the MetricsClient for this namespace | ||
| return new MetricsClient( | ||
| metricsEndpoint, | ||
| credential!, | ||
| new MetricsClientOptions | ||
| { | ||
| Audience = metricsClientAudience, | ||
| Transport = new HttpClientTransport( | ||
| new HttpClient(new SocketsHttpHandler | ||
| { | ||
| PooledConnectionIdleTimeout = TimeSpan.FromMinutes(2) | ||
| })) | ||
| }); | ||
| } | ||
|
|
||
| static Uri BuildMetricsEndpoint(MetricsClientAudience audience, string regionName) | ||
| { | ||
| var region = regionName.ToLowerInvariant(); | ||
| var builder = new UriBuilder(audience.ToString()); | ||
| builder.Host = $"{region}.{builder.Host}"; | ||
| return builder.Uri; | ||
| } | ||
|
|
||
| async Task<IReadOnlyList<MetricValue>> GetMetrics(string queueName, DateOnly startTime, DateOnly endTime, | ||
| async Task<IReadOnlyList<MonitorMetricValue>> GetMetrics(string queueName, DateOnly startTime, DateOnly endTime, | ||
| CancellationToken cancellationToken = default) | ||
| { | ||
| metricsClient ??= await InitializeMetricsClient(cancellationToken); | ||
|
|
||
| var response = await metricsClient.QueryResourcesAsync( | ||
| [resourceId!], | ||
| [CompleteMessageMetricName], | ||
| MicrosoftServicebusNamespacesMetricsNamespace, | ||
| new MetricsQueryResourcesOptions | ||
| { | ||
| Filter = $"EntityName eq '{queueName}'", | ||
| TimeRange = new MetricsQueryTimeRange(startTime.ToDateTime(TimeOnly.MinValue, DateTimeKind.Utc), endTime.ToDateTime(TimeOnly.MaxValue, DateTimeKind.Utc)), | ||
| Granularity = TimeSpan.FromDays(1) | ||
| }, | ||
| cancellationToken); | ||
|
|
||
| var metricQueryResult = response.Value.Values.SingleOrDefault(mr => mr.Namespace == MicrosoftServicebusNamespacesMetricsNamespace); | ||
|
|
||
| if (metricQueryResult is null) | ||
| var options = new ArmResourceGetMonitorMetricsOptions() | ||
| { | ||
| throw new Exception($"No metrics query results returned for {MicrosoftServicebusNamespacesMetricsNamespace}"); | ||
| } | ||
| Metricnames = CompleteMessageMetricName, | ||
| Timespan = $"{startTime:o}/{endTime:o}", | ||
| Filter = $"EntityName eq '{queueName}'", | ||
| Interval = TimeSpan.FromDays(1), | ||
| Metricnamespace = MicrosoftServicebusNamespacesMetricsNamespace | ||
| }; | ||
|
|
||
| var metricResult = metricQueryResult.GetMetricByName(CompleteMessageMetricName); | ||
|
|
||
| if (metricResult.Error.Message is not null) | ||
| await foreach (var metric in armClient.GetMonitorMetricsAsync(resourceId, options, cancellationToken)) | ||
| { | ||
| throw new Exception($"Metrics query result for '{metricResult.Name}' failed: {metricResult.Error.Message}"); | ||
| } | ||
|
|
||
| var timeSeries = metricResult.TimeSeries.SingleOrDefault(); | ||
|
|
||
| if (timeSeries is null) | ||
| { | ||
| throw new Exception($"Metrics query result for '{metricResult.Name}' contained no time series"); | ||
| foreach (var timeSeries in metric.Timeseries) | ||
| { | ||
| return timeSeries.Data; | ||
| } | ||
| } | ||
|
|
||
| return timeSeries.Values.AsReadOnly(); | ||
| // TODO: Better error handling | ||
|
Contributor
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. TODO |
||
| throw new Exception("No data returned from metrics query"); | ||
| } | ||
|
|
||
| public override async IAsyncEnumerable<IBrokerQueue> GetQueueNames( | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
this doesn't seem right? only returning the first timeseries data for the first metric?
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'll check but I believe there is only one metric, with one timeseries in it.