ILogger integration fix for net6 when WebApplicationBuilder is used
#3003
Conversation
This file contains 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
|
Looking into CI failure for |
pjanotti
reviewed
Oct 12, 2023
src/OpenTelemetry.AutoInstrumentation/Logger/LogBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.AutoInstrumentation/Logger/LogBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.AutoInstrumentation/Logger/LogBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.AutoInstrumentation/Logger/LogBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
I ran smoke test when bytecode instrumentation was not enabled, and it still crashed. It seems to be connected to addition of new kind of test app, I think this could be fixed as a part of separate issue. For now, I skipped problematic scenario. |
src/OpenTelemetry.AutoInstrumentation/Logger/LogBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
rajkumar-rangaraj
approved these changes
Oct 17, 2023
Kielek
approved these changes
Oct 18, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Why
Fixes #2994
What
Logs are exported multiple times for ASP.NET Core apps targeting
net6whenWebApplicationBuilderis used.In such scenario, integration is called on 2 related service collections:
WebApplicationServiceCollectionandServiceCollection.Integration tries to detect if it was called before on given
IServiceCollectioninstance, but it doesn't work in this case, as it is called onServiceCollectioninstance before services are copied to it fromWebApplicationServiceCollectioninstance (on which integration was called before). This results in same logs being exported 4 times (duplicated loggers with duplicated processors).Before #2975, integration was not being called if
builder.Serviceswas not ofServiceCollectiontype. That caused an issue whenHostingStartupAssemblywas configured to enable the integration (and bytecode instrumentation was disabled) for apps targetingnet6whenWebApplicationBuilderwas used.The fix checks if
HostingStartupAssemblyis configured, and doesn't run integration fornet6whenWebApplicationBuilderis used on host'sServiceCollectioninstance (OpenTelemetryLoggerrelated services will be copied fromWebApplicationServiceCollectioninstance, for which integration was called fromHostingStartupAssembly).All of the problematic scenarios (one that didn't work before #2975 and two that are being fixed with the PR) are covered by tests added in this PR.
Note:
As tests are waiting fixed amount of time to verify logs were not exported multiple times, this adds some time to the CI.
@rajkumar-rangaraj PTAL
Tests
Included in PR.
Checklist
CHANGELOG.mdis updated.- [ ] Documentation is updated.