Johluo/migrate more extensions logging #20421
Merged
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
Prior to reorganization, this source code was found in https://github.com/aspnet/Logging/tree/8270c545224e8734d7297e54edef5c584ee82f01
Prior to reorganization, this source code was found in https://github.com/aspnet/Logging/tree/5381f42ded1f41a3b0960bba799aed53da411401
Prior to reorganization, this source code was found in https://github.com/aspnet/Logging/tree/f7d8e4e0537eaab54dcf28c2b148b82688a3d62d
…maestro-bot/Common into merge/release/2.1-to-release/2.2
Addresses #571 Using `TaskCreationOptions.LongRunning` creates a dedicated thread, but it's released on the first await. Using Task.Run uses the thread pool instead
* Add attribute for collecting dumps for failed LoggedTest
.NET Core 2.0 reached EOL last year. This removes multi-targeting our test projects and test assets to only use .NET Core 2.1 and .NET Framework 4.6.1.
Use arcade
* Remove obsolete targets, properties, and scripts * Replace IsProductComponent with IsShipping * Undo bad merge to version.props * Update documentation, and put workarounds into a common file * Replace usages of RepositoryRoot with RepoRoot * Remove API baselines * Remove unnecessary restore feeds and split workarounds into two files * Enable PR checks on all branches, and disable autocancel
* Deprecate and replace IHostingEnvronment & IApplicationLifetime #966 * Fix startvs * Fix ref generation for obosolete
* Manualy update tooling dependencies to normalize names * Nullable
* Use Arcade's convention for setting IsPackable (must be explicitly set) * Use Arcade conventions for using DebugType and eng/Versions.props * Remove dead code * Update restore feeds in daily builds.md * Disable UsingToolNetFrameworkReferenceAssemblies in analyzer tests * Remove usage of TestGroupName (an obsolete KoreBuild setting) * Use IVT as a .csproj attribute
JunTaoLuo
added
the
area-infrastructure
wtgodbe
reviewed
Apr 1, 2020
src/Logging/Logging.AzureAppServices/src/Microsoft.Extensions.Logging.AzureAppServices.csproj
Outdated
Show resolved
Hide resolved
dougbu
reviewed
Apr 1, 2020
| { | ||
| var beforeCount = collection.Count; | ||
| collection.TryAddEnumerable(descriptor); | ||
| return beforeCount != collection.Count; |
There was a problem hiding this comment.
Since other threads can change the Count of this shared collection, suggest instead doing something like what TryAddEnumerable(...) does:
var implementationType = descriptor.GetImplementationType();
if (!services.Any(d =>
d.ServiceType == descriptor.ServiceType &&
d.GetImplementationType() == implementationType))
{
collection.TryAddEnumerable(descriptor);
return true;
}
return false;There was a problem hiding this comment.
Can you file a bug for this? I'm concerned with migrating files here, I'm not going to fix bugs as part of this PR.
src/Logging/Logging.Testing/src/Microsoft.Extensions.Logging.Testing.csproj
Outdated
Show resolved
Hide resolved
src/Logging/Logging.AzureAppServices/src/Microsoft.Extensions.Logging.AzureAppServices.csproj
Outdated
Show resolved
Hide resolved
|
No new C#, the only change I made was in 4294861 |
JunTaoLuo
requested review from
analogrelay,
BrennanConroy,
halter73,
jkotalik,
SteveSandersonMS and
Tratcher
as code owners
JunTaoLuo
removed request for
analogrelay,
halter73,
SteveSandersonMS,
Tratcher,
BrennanConroy and
jkotalik
JunTaoLuo
force-pushed
the
johluo/migrate-more-extensions-logging
branch
from
df307b4
to
f4a4346
Compare
|
Hmm agent down: |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
@dotnet/aspnet-build This is ready to go in FYI. |
dougbu
approved these changes
Apr 3, 2020
| { | ||
| var beforeCount = collection.Count; | ||
| collection.TryAddEnumerable(descriptor); | ||
| return beforeCount != collection.Count; |
There was a problem hiding this comment.
Reminder to self: File bug about reliability of this code.
| <PropertyGroup> | ||
| <Description>Logger implementation to support Azure App Services 'Diagnostics logs' and 'Log stream' features.</Description> | ||
| <TargetFramework>netstandard2.0</TargetFramework> | ||
| <NoWarn>$(NoWarn);CS1591</NoWarn> |
There was a problem hiding this comment.
We have <WarningsNotAsErrors>$(WarningsNotAsErrors);CS1591</WarningsNotAsErrors> in the root Directory.Build.props. We also have <NoWarn>$(NoWarn);CS1591</NoWarn> in >70 projects. Is this a declaration that we'll never get around to adding XML docs?
➕ @anurse because it looks like you own a lot of those 70+ projects.
| @@ -89,7 +89,7 @@ private async Task ClientCertTest(TestVariant variant, bool sendClientCert) | |||
| } | |||
| catch (Exception ex) | |||
| { | |||
| Logger.LogError($"Certificate is invalid. Issuer name: {cert.Issuer}"); | |||
| Logger.LogError($"Certificate is invalid. Issuer name: {cert?.Issuer}"); | |||
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.
Moving a few more things from extensions to aspnetcore to help remove dependencies on that repo. #19254
FYI only the last commit is interesting