Skip to content
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

Some Azure.Monitor.OpenTelemetry.Exporter tests fail under net7.0 #33403

Closed
heaths opened this issue Jan 10, 2023 · 6 comments · Fixed by #33554
Closed

Some Azure.Monitor.OpenTelemetry.Exporter tests fail under net7.0 #33403

heaths opened this issue Jan 10, 2023 · 6 comments · Fixed by #33554
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. Monitor - Exporter Monitor OpenTelemetry Exporter

Comments

@heaths
Copy link
Member

heaths commented Jan 10, 2023

When testing #32814 I found a few tests for Azure.Monitor.OpenTelemetry.Exporter are failing only under net7.0 with an error like:

Error message

System.FormatException : The format of value '' is invalid.

Stack trace

at System.Net.Http.Headers.HttpHeaderParser.ParseValue(String value, Object storeValue, Int32& index)
at System.Net.Http.Headers.HttpHeaders.ParseAndAddValue(HeaderDescriptor descriptor, HeaderStoreItemInfo info, String value)
at System.Net.Http.Headers.HttpHeaders.Add(HeaderDescriptor descriptor, String value)
at Microsoft.AspNetCore.Mvc.Testing.Handlers.CookieContainerHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
at Microsoft.AspNetCore.Mvc.Testing.Handlers.RedirectHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
at System.Net.Http.HttpClient.g__Core|83_0(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationTokenSource cts, Boolean disposeCts, CancellationTokenSource pendingRequestsCts, CancellationToken originalCancellationToken)
at Azure.Monitor.OpenTelemetry.Exporter.Integration.Tests.RequestTelemetryTests.VerifyRequestTelemetry() in /mnt/vss/_work/1/s/sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/tests/Integration.Tests/RequestTelemetryTests.cs:line 58
--- End of stack trace from previous location ---

From similar reports on GitHub, this most often seems to be a case where code assumed some value it's adding to headers was non-empty, and the fix has been to make sure the value to add (perhaps part of a header value collection) isn't empty. However, during initial investigation I found no code in sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src that could convevably add an empty header name or value, nor anything in the related tests folder. I fear this may be some middleware that hopefully just needs to be updated.

@heaths heaths added Client This issue points to a problem in the data-plane of the library. Monitor - Exporter Monitor OpenTelemetry Exporter labels Jan 10, 2023
@TimothyMothra
Copy link
Contributor

@heaths you mentioned a few tests, but only quoted the one "VerifyRequestTelemetry".
I understand you've been working on the net7 support for a while now. I'm okay with disabling our failing tests to unblock your effort.

CC @rajkumar-rangaraj

@heaths
Copy link
Member Author

heaths commented Jan 11, 2023

Tests include:

  • Azure.Monitor.OpenTelemetry.Exporter.Integration.Tests.BasicTests.VerifyRequest
  • Azure.Monitor.OpenTelemetry.Exporter.Integration.Tests.RequestTelemetryTests.VerifyRequestTelemetry

heaths added a commit to heaths/azure-sdk-for-net that referenced this issue Jan 11, 2023
@TimothyMothra
Copy link
Contributor

These are the integration tests; the unit test invokes a request to a web app and verify that a valid RequestTelemetry is generated.

These tests are failing because the request fails to invoke. Tests fail on this line: var response = await client.GetAsync(request);.

This affects net7 only, and the fix is to update the dependency:

  <ItemGroup Condition="'$(TargetFramework)' == 'net7.0'">
    <PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" VersionOverride="7.0.0" />
  </ItemGroup>

I want to avoid conflicts, so I'll wait for Heath's PR to merge and then I'll submit a PR with the fix. :)

@heaths
Copy link
Member Author

heaths commented Jan 11, 2023

I plan to merge EOW this week or early next week after our normal shipping cycle.

@heaths
Copy link
Member Author

heaths commented Jan 11, 2023

Please be sure to remove the Skip where I ignored those two tests.

@pallavit pallavit changed the title Some Azure.Monitor.OpenTelemetry.Exporter fail under net7.0 Some Azure.Monitor.OpenTelemetry.Exporter tests fail under net7.0 Jan 11, 2023
heaths added a commit that referenced this issue Jan 13, 2023
* Drop netcoreapp3.1, add net7.0 support

Resolves #32596. With netcoreapp3.1 falling out of support soon, we're upgrading to net7.0, net6.0, and, on Windows, net461.

* Resolve PR comments and fix build break

* Tweak platform monikers

* Upgrade to .NET 7.0.101 SDK

Required for microsoft/vstest#4014 fix

* Drop unnecessary runtimes from .vsconfig

* Skip Monitor tests failing under net7.0

Relates to #33403

* Resolve track 1 management plane issues on net7.0

* Resolve Key Vault issues after upgrade to net7.0

* Use different platform guard for management plane

* Resolve PR feedback
@heaths
Copy link
Member Author

heaths commented Jan 13, 2023

@TimothyMothra my PR to build tests for net7.0;net6.0[;net461] is now merged.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. Monitor - Exporter Monitor OpenTelemetry Exporter
Development

Successfully merging a pull request may close this issue.

5 participants