From 8b54074c7a392b0e295fa7aa0f1a3ab1cb28025a Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Thu, 1 Feb 2024 12:36:55 -0800 Subject: [PATCH 1/6] [prometheus-httplistener] Throw an exception if the listener cannot be started (#5304) --- .../CHANGELOG.md | 8 +- .../PrometheusHttpListener.cs | 6 +- ...enerMeterProviderBuilderExtensionsTests.cs | 2 +- .../PrometheusHttpListenerTests.cs | 89 ++++++++++++++++--- 4 files changed, 87 insertions(+), 18 deletions(-) diff --git a/src/OpenTelemetry.Exporter.Prometheus.HttpListener/CHANGELOG.md b/src/OpenTelemetry.Exporter.Prometheus.HttpListener/CHANGELOG.md index aa2981a5e16..279e18d90c2 100644 --- a/src/OpenTelemetry.Exporter.Prometheus.HttpListener/CHANGELOG.md +++ b/src/OpenTelemetry.Exporter.Prometheus.HttpListener/CHANGELOG.md @@ -2,11 +2,17 @@ ## Unreleased -* Export OpenMetrics format from Prometheus exporters ([#5107](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5107)) +* Export OpenMetrics format from Prometheus exporters + ([#5107](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5107)) + * For requests with OpenMetrics format, scope info is automatically added ([#5086](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5086) [#5182](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5182)) +* **Breaking change** Updated the `PrometheusHttpListener` to throw an exception + if it can't be started. + ([#5304](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5304)) + ## 1.7.0-rc.1 Released 2023-Nov-29 diff --git a/src/OpenTelemetry.Exporter.Prometheus.HttpListener/PrometheusHttpListener.cs b/src/OpenTelemetry.Exporter.Prometheus.HttpListener/PrometheusHttpListener.cs index beee72bc0d1..6f9663ba429 100644 --- a/src/OpenTelemetry.Exporter.Prometheus.HttpListener/PrometheusHttpListener.cs +++ b/src/OpenTelemetry.Exporter.Prometheus.HttpListener/PrometheusHttpListener.cs @@ -59,6 +59,8 @@ public void Start(CancellationToken token = default) return; } + this.httpListener.Start(); + // link the passed in token if not null this.tokenSource = token == default ? new CancellationTokenSource() : @@ -91,7 +93,7 @@ public void Dispose() { this.Stop(); - if (this.httpListener != null && this.httpListener.IsListening) + if (this.httpListener.IsListening) { this.httpListener.Close(); } @@ -111,8 +113,6 @@ private static bool AcceptsOpenMetrics(HttpListenerRequest request) private void WorkerProc() { - this.httpListener.Start(); - try { using var scope = SuppressInstrumentationScope.Begin(); diff --git a/test/OpenTelemetry.Exporter.Prometheus.HttpListener.Tests/PrometheusHttpListenerMeterProviderBuilderExtensionsTests.cs b/test/OpenTelemetry.Exporter.Prometheus.HttpListener.Tests/PrometheusHttpListenerMeterProviderBuilderExtensionsTests.cs index 882bbb7d024..af08063e267 100644 --- a/test/OpenTelemetry.Exporter.Prometheus.HttpListener.Tests/PrometheusHttpListenerMeterProviderBuilderExtensionsTests.cs +++ b/test/OpenTelemetry.Exporter.Prometheus.HttpListener.Tests/PrometheusHttpListenerMeterProviderBuilderExtensionsTests.cs @@ -23,7 +23,7 @@ public void TestAddPrometheusHttpListener_NamedOptions() services.Configure("Exporter2", o => namedExporterOptionsConfigureOptionsInvocations++); }) .AddPrometheusHttpListener() - .AddPrometheusHttpListener("Exporter2", o => { }) + .AddPrometheusHttpListener("Exporter2", o => o.ScrapeEndpointPath = "/metrics2") .Build(); Assert.Equal(1, defaultExporterOptionsConfigureOptionsInvocations); diff --git a/test/OpenTelemetry.Exporter.Prometheus.HttpListener.Tests/PrometheusHttpListenerTests.cs b/test/OpenTelemetry.Exporter.Prometheus.HttpListener.Tests/PrometheusHttpListenerTests.cs index 2d0271d84e1..ce7286d8c43 100644 --- a/test/OpenTelemetry.Exporter.Prometheus.HttpListener.Tests/PrometheusHttpListenerTests.cs +++ b/test/OpenTelemetry.Exporter.Prometheus.HttpListener.Tests/PrometheusHttpListenerTests.cs @@ -29,9 +29,7 @@ public class PrometheusHttpListenerTests [InlineData("http://example.com")] public void UriPrefixesPositiveTest(params string[] uriPrefixes) { - using MeterProvider meterProvider = Sdk.CreateMeterProviderBuilder() - .AddPrometheusHttpListener(options => options.UriPrefixes = uriPrefixes) - .Build(); + TestPrometheusHttpListenerUriPrefixOptions(uriPrefixes); } [Fact] @@ -39,9 +37,7 @@ public void UriPrefixesNull() { Assert.Throws(() => { - using MeterProvider meterProvider = Sdk.CreateMeterProviderBuilder() - .AddPrometheusHttpListener(options => options.UriPrefixes = null) - .Build(); + TestPrometheusHttpListenerUriPrefixOptions(null); }); } @@ -50,20 +46,16 @@ public void UriPrefixesEmptyList() { Assert.Throws(() => { - using MeterProvider meterProvider = Sdk.CreateMeterProviderBuilder() - .AddPrometheusHttpListener(options => options.UriPrefixes = new string[] { }) - .Build(); + TestPrometheusHttpListenerUriPrefixOptions(new string[] { }); }); } [Fact] public void UriPrefixesInvalid() { - Assert.Throws(() => + Assert.Throws(() => { - using MeterProvider meterProvider = Sdk.CreateMeterProviderBuilder() - .AddPrometheusHttpListener(options => options.UriPrefixes = new string[] { "ftp://example.com" }) - .Build(); + TestPrometheusHttpListenerUriPrefixOptions(new string[] { "ftp://example.com" }); }); } @@ -91,6 +83,77 @@ public async Task PrometheusExporterHttpServerIntegration_UseOpenMetricsVersionH await this.RunPrometheusExporterHttpServerIntegrationTest(acceptHeader: "application/openmetrics-text; version=1.0.0"); } + [Fact] + public void PrometheusHttpListenerThrowsOnStart() + { + Random random = new Random(); + int retryAttempts = 5; + int port = 0; + string address = null; + + PrometheusExporter exporter = null; + PrometheusHttpListener listener = null; + + // Step 1: Start a listener on a random port. + while (retryAttempts-- != 0) + { + port = random.Next(2000, 5000); + address = $"http://localhost:{port}/"; + + try + { + exporter = new PrometheusExporter(new()); + listener = new PrometheusHttpListener( + exporter, + new() + { + UriPrefixes = new string[] { address }, + }); + + listener.Start(); + + break; + } + catch + { + // ignored + } + } + + if (retryAttempts == 0) + { + throw new InvalidOperationException("PrometheusHttpListener could not be started"); + } + + // Step 2: Make sure if we start a second listener on the same port an exception is thrown. + Assert.Throws(() => + { + using var exporter = new PrometheusExporter(new()); + using var listener = new PrometheusHttpListener( + exporter, + new() + { + UriPrefixes = new string[] { address }, + }); + + listener.Start(); + }); + + exporter?.Dispose(); + listener?.Dispose(); + } + + private static void TestPrometheusHttpListenerUriPrefixOptions(string[] uriPrefixes) + { + using var exporter = new PrometheusExporter(new()); + using var listener = new PrometheusHttpListener( + exporter, + new() + { + UriPrefixes = uriPrefixes, + }); + } + private async Task RunPrometheusExporterHttpServerIntegrationTest(bool skipMetrics = false, string acceptHeader = "application/openmetrics-text") { var requestOpenMetrics = acceptHeader.StartsWith("application/openmetrics-text"); From c28b3e12516f6a05727a6d63ba6756a195fbca41 Mon Sep 17 00:00:00 2001 From: Alan West <3676547+alanwest@users.noreply.github.com> Date: Thu, 1 Feb 2024 14:49:53 -0800 Subject: [PATCH 2/6] [aspnetcore-route-docs] Fix markdown to make markdownlint v15 happy (#5306) Co-authored-by: Mikel Blanchard --- .../RouteTests/README.net6.0.md | 12 +++---- .../RouteTests/README.net7.0.md | 12 +++---- .../RouteTests/README.net8.0.md | 32 +++++++++++++++---- .../RouteTests/RoutingTestCases.json | 6 ++-- .../RouteTests/RoutingTestFixture.cs | 8 +++-- 5 files changed, 46 insertions(+), 24 deletions(-) diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net6.0.md b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net6.0.md index d2c3b5b29f7..6582c757155 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net6.0.md +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net6.0.md @@ -8,9 +8,9 @@ | :green_heart: | ConventionalRouting | [Not Found (404)](#conventionalrouting-not-found-404) | | :green_heart: | ConventionalRouting | [Route template with parameter constraint](#conventionalrouting-route-template-with-parameter-constraint) | | :green_heart: | ConventionalRouting | [Path that does not match parameter constraint](#conventionalrouting-path-that-does-not-match-parameter-constraint) | -| :broken_heart: | ConventionalRouting | [Area using area:exists, default controller/action](#conventionalrouting-area-using-areaexists-default-controlleraction) | -| :broken_heart: | ConventionalRouting | [Area using area:exists, non-default action](#conventionalrouting-area-using-areaexists-non-default-action) | -| :broken_heart: | ConventionalRouting | [Area w/o area:exists, default controller/action](#conventionalrouting-area-wo-areaexists-default-controlleraction) | +| :broken_heart: | ConventionalRouting | [Area using `area:exists`, default controller/action](#conventionalrouting-area-using-areaexists-default-controlleraction) | +| :broken_heart: | ConventionalRouting | [Area using `area:exists`, non-default action](#conventionalrouting-area-using-areaexists-non-default-action) | +| :broken_heart: | ConventionalRouting | [Area w/o `area:exists`, default controller/action](#conventionalrouting-area-wo-areaexists-default-controlleraction) | | :green_heart: | AttributeRouting | [Default action](#attributerouting-default-action) | | :green_heart: | AttributeRouting | [Action without parameter](#attributerouting-action-without-parameter) | | :green_heart: | AttributeRouting | [Action with parameter](#attributerouting-action-with-parameter) | @@ -194,7 +194,7 @@ } ``` -## ConventionalRouting: Area using area:exists, default controller/action +## ConventionalRouting: Area using `area:exists`, default controller/action ```json { @@ -225,7 +225,7 @@ } ``` -## ConventionalRouting: Area using area:exists, non-default action +## ConventionalRouting: Area using `area:exists`, non-default action ```json { @@ -256,7 +256,7 @@ } ``` -## ConventionalRouting: Area w/o area:exists, default controller/action +## ConventionalRouting: Area w/o `area:exists`, default controller/action ```json { diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net7.0.md b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net7.0.md index 35a0e331882..49d8224155c 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net7.0.md +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net7.0.md @@ -8,9 +8,9 @@ | :green_heart: | ConventionalRouting | [Not Found (404)](#conventionalrouting-not-found-404) | | :green_heart: | ConventionalRouting | [Route template with parameter constraint](#conventionalrouting-route-template-with-parameter-constraint) | | :green_heart: | ConventionalRouting | [Path that does not match parameter constraint](#conventionalrouting-path-that-does-not-match-parameter-constraint) | -| :broken_heart: | ConventionalRouting | [Area using area:exists, default controller/action](#conventionalrouting-area-using-areaexists-default-controlleraction) | -| :broken_heart: | ConventionalRouting | [Area using area:exists, non-default action](#conventionalrouting-area-using-areaexists-non-default-action) | -| :broken_heart: | ConventionalRouting | [Area w/o area:exists, default controller/action](#conventionalrouting-area-wo-areaexists-default-controlleraction) | +| :broken_heart: | ConventionalRouting | [Area using `area:exists`, default controller/action](#conventionalrouting-area-using-areaexists-default-controlleraction) | +| :broken_heart: | ConventionalRouting | [Area using `area:exists`, non-default action](#conventionalrouting-area-using-areaexists-non-default-action) | +| :broken_heart: | ConventionalRouting | [Area w/o `area:exists`, default controller/action](#conventionalrouting-area-wo-areaexists-default-controlleraction) | | :green_heart: | AttributeRouting | [Default action](#attributerouting-default-action) | | :green_heart: | AttributeRouting | [Action without parameter](#attributerouting-action-without-parameter) | | :green_heart: | AttributeRouting | [Action with parameter](#attributerouting-action-with-parameter) | @@ -196,7 +196,7 @@ } ``` -## ConventionalRouting: Area using area:exists, default controller/action +## ConventionalRouting: Area using `area:exists`, default controller/action ```json { @@ -227,7 +227,7 @@ } ``` -## ConventionalRouting: Area using area:exists, non-default action +## ConventionalRouting: Area using `area:exists`, non-default action ```json { @@ -258,7 +258,7 @@ } ``` -## ConventionalRouting: Area w/o area:exists, default controller/action +## ConventionalRouting: Area w/o `area:exists`, default controller/action ```json { diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net8.0.md b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net8.0.md index 3b712f73e20..40b63a1ca45 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net8.0.md +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net8.0.md @@ -8,9 +8,9 @@ | :green_heart: | ConventionalRouting | [Not Found (404)](#conventionalrouting-not-found-404) | | :green_heart: | ConventionalRouting | [Route template with parameter constraint](#conventionalrouting-route-template-with-parameter-constraint) | | :green_heart: | ConventionalRouting | [Path that does not match parameter constraint](#conventionalrouting-path-that-does-not-match-parameter-constraint) | -| :broken_heart: | ConventionalRouting | [Area using area:exists, default controller/action](#conventionalrouting-area-using-areaexists-default-controlleraction) | -| :broken_heart: | ConventionalRouting | [Area using area:exists, non-default action](#conventionalrouting-area-using-areaexists-non-default-action) | -| :broken_heart: | ConventionalRouting | [Area w/o area:exists, default controller/action](#conventionalrouting-area-wo-areaexists-default-controlleraction) | +| :broken_heart: | ConventionalRouting | [Area using `area:exists`, default controller/action](#conventionalrouting-area-using-areaexists-default-controlleraction) | +| :broken_heart: | ConventionalRouting | [Area using `area:exists`, non-default action](#conventionalrouting-area-using-areaexists-non-default-action) | +| :broken_heart: | ConventionalRouting | [Area w/o `area:exists`, default controller/action](#conventionalrouting-area-wo-areaexists-default-controlleraction) | | :green_heart: | AttributeRouting | [Default action](#attributerouting-default-action) | | :green_heart: | AttributeRouting | [Action without parameter](#attributerouting-action-without-parameter) | | :green_heart: | AttributeRouting | [Action with parameter](#attributerouting-action-with-parameter) | @@ -24,6 +24,7 @@ | :green_heart: | MinimalApi | [Action with parameter](#minimalapi-action-with-parameter) | | :green_heart: | MinimalApi | [Action without parameter (MapGroup)](#minimalapi-action-without-parameter-mapgroup) | | :green_heart: | MinimalApi | [Action with parameter (MapGroup)](#minimalapi-action-with-parameter-mapgroup) | +| :green_heart: | ExceptionMiddleware | [Exception Handled by Exception Handler Middleware](#exceptionmiddleware-exception-handled-by-exception-handler-middleware) | ## ConventionalRouting: Root path @@ -195,7 +196,7 @@ } ``` -## ConventionalRouting: Area using area:exists, default controller/action +## ConventionalRouting: Area using `area:exists`, default controller/action ```json { @@ -226,7 +227,7 @@ } ``` -## ConventionalRouting: Area using area:exists, non-default action +## ConventionalRouting: Area using `area:exists`, non-default action ```json { @@ -257,7 +258,7 @@ } ``` -## ConventionalRouting: Area w/o area:exists, default controller/action +## ConventionalRouting: Area w/o `area:exists`, default controller/action ```json { @@ -632,3 +633,22 @@ } } ``` + +## ExceptionMiddleware: Exception Handled by Exception Handler Middleware + +```json +{ + "IdealHttpRoute": "/Exception", + "ActivityDisplayName": "GET /Exception", + "ActivityHttpRoute": "/Exception", + "MetricHttpRoute": "/Exception", + "RouteInfo": { + "HttpMethod": "GET", + "Path": "/Exception", + "RoutePattern.RawText": "/Exception", + "IRouteDiagnosticsMetadata.Route": "/Exception", + "HttpContext.GetRouteData()": {}, + "ActionDescriptor": null + } +} +``` diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestCases.json b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestCases.json index 2d7dac9727a..2d1fa584ee8 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestCases.json +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestCases.json @@ -54,7 +54,7 @@ "expectedHttpRoute": "" }, { - "name": "Area using area:exists, default controller/action", + "name": "Area using `area:exists`, default controller/action", "testApplicationScenario": "ConventionalRouting", "httpMethod": "GET", "path": "/MyArea", @@ -63,7 +63,7 @@ "expectedHttpRoute": "{area:exists}/ControllerForMyArea/Default/{id?}" }, { - "name": "Area using area:exists, non-default action", + "name": "Area using `area:exists`, non-default action", "testApplicationScenario": "ConventionalRouting", "httpMethod": "GET", "path": "/MyArea/ControllerForMyArea/NonDefault", @@ -72,7 +72,7 @@ "expectedHttpRoute": "{area:exists}/ControllerForMyArea/NonDefault/{id?}" }, { - "name": "Area w/o area:exists, default controller/action", + "name": "Area w/o `area:exists`, default controller/action", "testApplicationScenario": "ConventionalRouting", "httpMethod": "GET", "path": "/SomePrefix", diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestFixture.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestFixture.cs index d22093f0158..1b949e26740 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestFixture.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestFixture.cs @@ -71,7 +71,7 @@ private void GenerateReadme() { var result = this.testResults[i]; var emoji = result.TestCase.CurrentHttpRoute == null ? ":green_heart:" : ":broken_heart:"; - sb.AppendLine($"| {emoji} | {result.TestCase.TestApplicationScenario} | [{result.TestCase.Name}]({MakeAnchorTag(result.TestCase.TestApplicationScenario, result.TestCase.Name)}) |"); + sb.AppendLine($"| {emoji} | {result.TestCase.TestApplicationScenario} | [{result.TestCase.Name}]({GenerateLinkFragment(result.TestCase.TestApplicationScenario, result.TestCase.Name)}) |"); } for (var i = 0; i < this.testResults.Count; ++i) @@ -88,10 +88,12 @@ private void GenerateReadme() var readmeFileName = $"README.net{Environment.Version.Major}.0.md"; File.WriteAllText(Path.Combine("..", "..", "..", "RouteTests", readmeFileName), sb.ToString()); - static string MakeAnchorTag(TestApplicationScenario scenario, string name) + // Generates a link fragment that should comply with markdownlint rule MD051 + // https://github.com/DavidAnson/markdownlint/blob/main/doc/md051.md + static string GenerateLinkFragment(TestApplicationScenario scenario, string name) { var chars = name.ToCharArray() - .Where(c => !char.IsPunctuation(c) || c == '-') + .Where(c => (!char.IsPunctuation(c) && c != '`') || c == '-') .Select(c => c switch { '-' => '-', From 4dedeade304c7d44c5a66ff1242ffac4073b2ae3 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 1 Feb 2024 14:59:21 -0800 Subject: [PATCH 3/6] Bump DavidAnson/markdownlint-cli2-action from 14.0.0 to 15.0.0 (#5290) Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Mikel Blanchard --- .github/workflows/markdownlint.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/markdownlint.yml b/.github/workflows/markdownlint.yml index 1a4a8b38b8c..462832652d5 100644 --- a/.github/workflows/markdownlint.yml +++ b/.github/workflows/markdownlint.yml @@ -14,7 +14,7 @@ jobs: uses: actions/checkout@v4 - name: run markdownlint - uses: DavidAnson/markdownlint-cli2-action@v14.0.0 + uses: DavidAnson/markdownlint-cli2-action@v15.0.0 with: globs: | **/*.md From edfea52f2e2021c4cdcb174499efe8207e3a52f2 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Thu, 1 Feb 2024 16:40:48 -0800 Subject: [PATCH 4/6] Bump codecov/codecov-action from 3.1.5 to 4.0.1 (#5294) --- .github/workflows/Component.BuildTest.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/Component.BuildTest.yml b/.github/workflows/Component.BuildTest.yml index c21b9c4aa2e..f97c8b94f9b 100644 --- a/.github/workflows/Component.BuildTest.yml +++ b/.github/workflows/Component.BuildTest.yml @@ -68,13 +68,15 @@ jobs: run: dotnet-coverage merge -r -f cobertura -o ./TestResults/Cobertura.xml ./TestResults/*.coverage - name: Upload code coverage ${{ inputs.code-cov-prefix }}-${{ inputs.code-cov-name }} - uses: codecov/codecov-action@v3.1.5 + uses: codecov/codecov-action@v4 continue-on-error: true # Note: Don't fail for upload failures env: OS: ${{ matrix.os }} TFM: ${{ matrix.version }} + token: ${{ secrets.CODECOV_TOKEN }} with: file: TestResults/Cobertura.xml env_vars: OS,TFM flags: ${{ inputs.code-cov-prefix }}-${{ inputs.code-cov-name }} name: Code Coverage for ${{ inputs.code-cov-prefix }}-${{ inputs.code-cov-name }} on [${{ matrix.os }}.${{ matrix.version }}] + codecov_yml_path: .github/codecov.yml From 71f9a4217f1ffed45c4a0043365ec8359b84f59c Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 2 Feb 2024 12:00:23 -0800 Subject: [PATCH 5/6] Bump al-cheb/configure-pagefile-action from 1.3 to 1.4 (#5309) Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/codeql-analysis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index ee706de7d0a..9cf1fe874c0 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -25,7 +25,7 @@ jobs: steps: - name: configure Pagefile - uses: al-cheb/configure-pagefile-action@v1.3 + uses: al-cheb/configure-pagefile-action@v1.4 with: minimum-size: 8GB maximum-size: 32GB From ef942a11ac264d7b0f754a78dd3b4dd7d4d2c6f3 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 2 Feb 2024 12:23:18 -0800 Subject: [PATCH 6/6] [metrics] Fix perf regression introduced by metric point reclaim feature (#5307) Co-authored-by: Alan West <3676547+alanwest@users.noreply.github.com> --- src/OpenTelemetry/Metrics/AggregatorStore.cs | 23 +++++++++----------- src/OpenTelemetry/Metrics/MetricPoint.cs | 14 +++++------- 2 files changed, 15 insertions(+), 22 deletions(-) diff --git a/src/OpenTelemetry/Metrics/AggregatorStore.cs b/src/OpenTelemetry/Metrics/AggregatorStore.cs index 1ca4baa1114..e181a81fb82 100644 --- a/src/OpenTelemetry/Metrics/AggregatorStore.cs +++ b/src/OpenTelemetry/Metrics/AggregatorStore.cs @@ -12,7 +12,7 @@ namespace OpenTelemetry.Metrics; internal sealed class AggregatorStore { internal readonly bool OutputDelta; - internal readonly bool ShouldReclaimUnusedMetricPoints; + internal readonly bool OutputDeltaWithUnusedMetricPointReclaimEnabled; internal long DroppedMeasurements = 0; private static readonly string MetricPointCapHitFixMessage = "Consider opting in for the experimental SDK feature to emit all the throttled metrics under the overflow attribute by setting env variable OTEL_DOTNET_EXPERIMENTAL_METRICS_EMIT_OVERFLOW_ATTRIBUTE = true. You could also modify instrumentation to reduce the number of unique key/value pair combinations. Or use Views to drop unwanted tags. Or use MeterProviderBuilder.SetMaxMetricPointsPerMetricStream to set higher limit."; @@ -101,9 +101,9 @@ internal AggregatorStore( reservedMetricPointsCount++; } - this.ShouldReclaimUnusedMetricPoints = shouldReclaimUnusedMetricPoints; + this.OutputDeltaWithUnusedMetricPointReclaimEnabled = shouldReclaimUnusedMetricPoints && this.OutputDelta; - if (this.OutputDelta && shouldReclaimUnusedMetricPoints) + if (this.OutputDeltaWithUnusedMetricPointReclaimEnabled) { this.availableMetricPoints = new Queue(maxMetricPoints - reservedMetricPointsCount); @@ -158,17 +158,14 @@ internal void Update(double value, ReadOnlySpan> t internal int Snapshot() { this.batchSize = 0; - if (this.OutputDelta) + if (this.OutputDeltaWithUnusedMetricPointReclaimEnabled) { - if (this.ShouldReclaimUnusedMetricPoints) - { - this.SnapshotDeltaWithMetricPointReclaim(); - } - else - { - var indexSnapshot = Math.Min(this.metricPointIndex, this.maxMetricPoints - 1); - this.SnapshotDelta(indexSnapshot); - } + this.SnapshotDeltaWithMetricPointReclaim(); + } + else if (this.OutputDelta) + { + var indexSnapshot = Math.Min(this.metricPointIndex, this.maxMetricPoints - 1); + this.SnapshotDelta(indexSnapshot); } else { diff --git a/src/OpenTelemetry/Metrics/MetricPoint.cs b/src/OpenTelemetry/Metrics/MetricPoint.cs index d66ef5b309f..349afd51bd9 100644 --- a/src/OpenTelemetry/Metrics/MetricPoint.cs +++ b/src/OpenTelemetry/Metrics/MetricPoint.cs @@ -50,11 +50,7 @@ internal MetricPoint( { Debug.Assert(aggregatorStore != null, "AggregatorStore was null."); Debug.Assert(histogramExplicitBounds != null, "Histogram explicit Bounds was null."); - - if (aggregatorStore!.OutputDelta && aggregatorStore.ShouldReclaimUnusedMetricPoints) - { - Debug.Assert(lookupData != null, "LookupData was null."); - } + Debug.Assert(!aggregatorStore!.OutputDeltaWithUnusedMetricPointReclaimEnabled || lookupData != null, "LookupData was null."); this.aggType = aggType; this.Tags = new ReadOnlyTagCollection(tagKeysAndValues); @@ -445,7 +441,7 @@ internal void Update(long number) // by ignoring Zero points this.MetricPointStatus = MetricPointStatus.CollectPending; - if (this.aggregatorStore.OutputDelta) + if (this.aggregatorStore.OutputDeltaWithUnusedMetricPointReclaimEnabled) { Interlocked.Decrement(ref this.ReferenceCount); } @@ -570,7 +566,7 @@ internal void UpdateWithExemplar(long number, ReadOnlySpan