Update logging with SDK model by amerjusupovic · Pull Request #367 · Azure/AppConfiguration-DotnetProvider

Amer Jusupovic (amerjusupovic) · 2022-11-21T17:34:44Z

No description provided.

…otnetProvider into ajusupovic/update-logging

Zhenlan Wang (zhenlan) · 2022-12-01T22:28:37Z

<TargetFramework>net5.0</TargetFramework>

net5.0 is out of support. We should update the example to net6.0 or net7.0. How about you do that in a separate PR including any dependency package update?

Refers to: examples/ConsoleAppWithFailOver/ConsoleAppWithFailOver.csproj:5 in a1d8039. [](commit_id = a1d8039, deletion_comment = False)

…lper class

Avani Gupta (avanigupta)

Can you add the Copyright header in all new files?

…otnetProvider into ajusupovic/update-logging

Avani Gupta (avanigupta) · 2023-03-15T19:53:08Z

Jimmy Campbell (@jimmyca15) Zhenlan Wang (@zhenlan) can you take another look at this PR so it can be closed?

Jimmy Campbell (jimmyca15) · 2023-03-15T21:50:49Z

+
+        public Logger(ILogger logger)
+        {
+            _logger = logger;


The expectation of this constructor is logger is not null right? If so, needs null check.

Jimmy Campbell (jimmyca15) · 2023-03-15T21:56:46Z

            else
            {
-                _logger.LogWarning($"Ignoring the push notification received for the unregistered endpoint '{pushNotification.ResourceUri}'");
+                _logger.LogWarning(LoggingConstants.PushNotificationUnregisteredEndpoint + pushNotification.ResourceUri + ".");


Some places use Build paradigm, some don't. What dictates the decision?

Also, it looks like LoggingConstants.PushNotificationUnregisteredEndpoint includes an opening ', which isn't closed here. Does look like a good fit for parameterized string with string.format.

Or the pattern established in LogHelper already.

Jimmy Campbell (jimmyca15) · 2023-03-15T21:58:06Z

            else
            {
-                _logger.LogWarning($"Ignoring the push notification received for the unregistered endpoint '{pushNotification.ResourceUri}'");
+                _logger.LogWarning(LoggingConstants.PushNotificationUnregisteredEndpoint + pushNotification.ResourceUri + ".");


Also, it looks like LoggingConstants.PushNotificationUnregisteredEndpoint includes an opening ', which isn't closed here. Does look like a good fit for parameterized string with string.format.

Jimmy Campbell (jimmyca15) · 2023-03-15T21:58:41Z

            else
            {
-                _logger.LogWarning($"Ignoring the push notification received for the unregistered endpoint '{pushNotification.ResourceUri}'");
+                _logger.LogWarning(LoggingConstants.PushNotificationUnregisteredEndpoint + pushNotification.ResourceUri + ".");


Or the pattern established in LogHelper already.

Amer Jusupovic (amerjusupovic) added 11 commits November 2, 2022 12:59

remove redacted from query params sdk logging

c78090c

fix query params

e9fbff6

test eventsource logging

5d5cd31

comment out listener

1d04d70

change event source, log statements, in progress

e494d5a

revert logger parameter in IKeyValueAdapter

23346d8

fix logwarning, in progress

2fe00e7

Merge branch 'preview' of https://github.com/Azure/AppConfiguration-D…

c070f1e

…otnetProvider into ajusupovic/update-logging

change tests, fix formatting in progress

210431b

upgrade azure identity version

38a655f

fix comments, consistency

0eefc00

Amer Jusupovic (amerjusupovic) marked this pull request as draft November 23, 2022 22:16

Amer Jusupovic (amerjusupovic) added 2 commits December 1, 2022 12:19

change message format, support ilogger and sdk logs

0638b40

fix exception log to match others

a1d8039

Zhenlan Wang (zhenlan) reviewed Dec 1, 2022

View reviewed changes

Comment thread src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationOptions.cs Outdated

Zhenlan Wang (zhenlan) reviewed Dec 1, 2022

View reviewed changes

Comment thread src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationOptions.cs Outdated

Zhenlan Wang (zhenlan) reviewed Dec 1, 2022

View reviewed changes

Comment thread src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs Outdated

Zhenlan Wang (zhenlan) reviewed Dec 1, 2022

View reviewed changes

Comment thread src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs Outdated

Zhenlan Wang (zhenlan) reviewed Dec 1, 2022

View reviewed changes

Comment thread src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs

Zhenlan Wang (zhenlan) reviewed Dec 1, 2022

View reviewed changes

Comment thread src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs Outdated

Amer Jusupovic (amerjusupovic) added 4 commits December 2, 2022 15:51

revert package versions, add helpers formatlog and handlelog

58228e1

fix non-examples packages

b03c6c3

fix identity version

798559f

fix variable type clientoptions

d1d3788

Amer Jusupovic (amerjusupovic) marked this pull request as ready for review December 5, 2022 18:57

Amer Jusupovic (amerjusupovic) marked this pull request as draft December 5, 2022 19:00

Amer Jusupovic (amerjusupovic) added 2 commits December 6, 2022 14:12

fix tests, change log format, name of source

2fe56a6

fix newline, rename event source

d6ef15b

Amer Jusupovic (amerjusupovic) marked this pull request as ready for review December 7, 2022 17:54

Avani Gupta (avanigupta) reviewed Jan 17, 2023

View reviewed changes

Comment thread ...sions.Configuration.AzureAppConfiguration/AzureAppConfigurationProviderRefreshEventSource.cs

Avani Gupta (avanigupta) reviewed Jan 17, 2023

View reviewed changes

Comment thread src/Microsoft.Extensions.Configuration.AzureAppConfiguration/Logger.cs

Avani Gupta (avanigupta) reviewed Jan 17, 2023

View reviewed changes

Comment thread src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs Outdated

Avani Gupta (avanigupta) reviewed Jan 17, 2023

View reviewed changes

Comment thread src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs Outdated

Avani Gupta (avanigupta) reviewed Jan 17, 2023

View reviewed changes

Comment thread src/Microsoft.Extensions.Configuration.AzureAppConfiguration/IKeyValueAdapter.cs

Amer Jusupovic (amerjusupovic) added 3 commits January 18, 2023 11:50

some revisions

c53f0a1

fix LogWarning to take only message param

f2ba02a

fix logging test

2a56ec2

Avani Gupta (avanigupta) reviewed Jan 26, 2023

View reviewed changes

Comment thread src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs Outdated

Avani Gupta (avanigupta) reviewed Jan 26, 2023

View reviewed changes

Comment thread ...sions.Configuration.AzureAppConfiguration/FeatureManagement/DynamicFeatureKeyValueAdapter.cs

Amer Jusupovic (amerjusupovic) added 2 commits January 26, 2023 13:55

remove unused usings, fix warning log msg

9b767dc

unused using

38dd1ed

Amer Jusupovic (amerjusupovic) requested a review from Avani Gupta (avanigupta) February 13, 2023 18:29

Avani Gupta (avanigupta) reviewed Feb 15, 2023

View reviewed changes

Comment thread src/Microsoft.Extensions.Configuration.AzureAppConfiguration/Logger.cs Outdated

Avani Gupta (avanigupta) reviewed Feb 15, 2023

View reviewed changes

Comment thread tests/Tests.AzureAppConfiguration/FeatureManagementTests.cs Outdated

make tests use build log methods, move build log methods to static he…

6afe8ce

…lper class

Avani Gupta (avanigupta) approved these changes Feb 15, 2023

View reviewed changes

Amer Jusupovic (amerjusupovic) added 2 commits February 16, 2023 15:00

add copyright headers

4ada765

Merge branch 'preview' of https://github.com/Azure/AppConfiguration-D…

c492ff4

…otnetProvider into ajusupovic/update-logging

Jimmy Campbell (jimmyca15) reviewed Mar 15, 2023

View reviewed changes

Jimmy Campbell (jimmyca15) approved these changes Mar 15, 2023

View reviewed changes

Amer Jusupovic (amerjusupovic) and others added 2 commits March 20, 2023 11:45

fix log pattern, add null check to logger constructor

03dd4a2

Merge branch 'preview' into ajusupovic/http-logging

130488f

Amer Jusupovic (amerjusupovic) merged commit 244b170 into preview Mar 21, 2023

Amer Jusupovic (amerjusupovic) deleted the ajusupovic/http-logging branch March 21, 2023 15:57

Amer Jusupovic (amerjusupovic) linked an issue Mar 29, 2023 that may be closed by this pull request

Add ability to perform custom transforms on App Configuration Settings. #157

Closed

Amer Jusupovic (amerjusupovic) removed a link to an issue Mar 29, 2023

Add ability to perform custom transforms on App Configuration Settings. #157

Closed

Amer Jusupovic (amerjusupovic) linked an issue Mar 29, 2023 that may be closed by this pull request

Consider adding logging for TryRefreshAsync method #134

Closed

Conversation

Amer Jusupovic (amerjusupovic) commented Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Zhenlan Wang (zhenlan) commented Dec 1, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Avani Gupta (avanigupta) left a comment

Choose a reason for hiding this comment

Uh oh!

Avani Gupta (avanigupta) commented Mar 15, 2023

Uh oh!

Jimmy Campbell (jimmyca15) Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

Jimmy Campbell (jimmyca15) Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

Jimmy Campbell (jimmyca15) Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

Jimmy Campbell (jimmyca15) Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

Jimmy Campbell (jimmyca15) Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

Jimmy Campbell (jimmyca15) Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Amer Jusupovic (amerjusupovic) commented Nov 21, 2022 •

edited

Loading