Add Blob AzureWebApp provider #477

pakrym · 2016-08-16T21:45:16Z

No description provided.

pakrym · 2016-08-17T15:51:41Z

davidfowl · 2016-08-17T16:02:19Z

src/Microsoft.Extensions.Logging.AzureWebAppDiagnostics/Internal/AzureBlobSink.cs

+                CloudBlobStream stream;
+                try
+                {
+                    stream = await blob.OpenWriteAsync(false);


false? What's the argument name

campersau · 2016-08-17T17:01:22Z

src/Microsoft.Extensions.Logging.AzureWebAppDiagnostics/AzureWebAppDiagnosticsLoggerProvider.cs

        }

        /// <inheritdoc />
        public void Dispose()
        {
-            _innerLoggerProvider.Dispose();
+            _loggerFactory.Dispose();


I think this can also be null.

victorhurdugaci · 2016-08-19T06:04:10Z

You didn't compile locally after your last changes, didn't you? 😏 (travis/appveyor)

pakrym · 2016-08-19T15:58:40Z

Don't tell anybody :) Also thank you for reviewing while on vacation.

cesarblum · 2016-08-19T20:01:56Z

src/Microsoft.Extensions.Logging.AzureWebAppDiagnostics/Internal/AzureBlobLoggerProvider.cs

+            var fileName = _instanceId + "-" + _fileName;
+            var azureBlobSink = new AzureBlobSink(container, _appName, fileName, messageFormatter, _batchSize, _period);
+            var backgroundSink = new BackgroundSink(azureBlobSink, BackgroundSink.DefaultLogMessagesQueueSize);
+            LoggerConfiguration loggerConfiguration = new LoggerConfiguration();


victorhurdugaci · 2016-08-25T16:33:03Z

src/Microsoft.Extensions.Logging.AzureWebAppDiagnostics/Internal/AzureBlobSink.cs

+            }
+            if (period <= TimeSpan.Zero)
+            {
+                throw new ArgumentOutOfRangeException(nameof(period), $"{nameof(period)} should be longer then zero.");


victorhurdugaci · 2016-08-25T16:39:41Z

The code looks okay but it would be nice to have some tests that actually check the azure sdk part

pakrym · 2016-08-25T23:01:23Z

I tries to write tests but it's nearly impossible to mock out azure blob types.

victorhurdugaci · 2016-08-26T15:45:15Z

You could try to use the emulator for tests or you might want to take a look at WebJobs. I remember there was some storage mocking there. If you get any code from there, make sure you sync with Eilon first - he was the manager for that project.

pakrym · 2016-08-29T17:35:11Z

@victorhurdugaci added tests.

victorhurdugaci · 2016-08-29T17:45:25Z

test/Microsoft.Extensions.Logging.AzureWebAppDiagnostics.Test/AzureBlobSinkTests.cs

+            {
+                logger.Information("Text " + i);
+            }
+            await Task.Delay(TimeSpan.FromSeconds(1));


no, no... use some sort of polling. Hardcoded wait times are evil

You can probably poll the buffer until you get the expected number of messages. (with a timeout, of course)

pakrym · 2016-08-29T19:12:33Z

@victorhurdugaci switched to CountdownEvent

victorhurdugaci · 2016-08-29T22:11:45Z

test/Microsoft.Extensions.Logging.AzureWebAppDiagnostics.Test/AzureBlobSinkTests.cs

+                logger.Information("Text " + i);
+            }
+
+            Assert.True(sink.CountdownEvent.Wait(TimeSpan.FromSeconds(1)));


Be more reasonable. Give it 10 seconds

victorhurdugaci · 2016-08-29T22:20:16Z

once Travis and AppVeyor pass

pakrym · 2016-08-30T15:52:37Z

@victorhurdugaci appveyor/pr fails with

Microsoft.Extensions.Logging.AzureWebAppDiagnostics.Test.LogConfigurationReaderTests.ConfigurationChange [FAIL]
      System.TimeoutException : Maximum number of retries reached.
      Stack Trace:
        C:\projects\logging\test\Microsoft.Extensions.Logging.AzureWebAppDiagnostics.Test\RetryHelper.cs(25,0): at Microsoft.Extensions.Logging.RetryHelper.Retry(Func`1 action, Int32 retries)
        C:\projects\logging\test\Microsoft.Extensions.Logging.AzureWebAppDiagnostics.Test\LogConfigurationReaderTests.cs(179,0): at Microsoft.Extensions.Logging.AzureWebAppDiagnostics.Test.LogConfigurationReaderTests.ConfigurationChange()
  Finished:    Microsoft.Extensions.Logging.AzureWebAppDiagnostics.Test

Which is unrelated to this PR. Merging.

dnfclas added the cla-already-signed label Aug 16, 2016

pakrym force-pushed the pakrym/blob branch from b195146 to cdfad9e Compare August 16, 2016 21:52

davidfowl reviewed Aug 17, 2016
View reviewed changes

pakrym force-pushed the pakrym/blob branch from cdfad9e to 2de4588 Compare August 17, 2016 16:24

campersau reviewed Aug 17, 2016
View reviewed changes

pakrym force-pushed the pakrym/blob branch from 9847000 to 092fc68 Compare August 18, 2016 23:27

pakrym force-pushed the pakrym/blob branch from 092fc68 to 158bfef Compare August 19, 2016 15:59

cesarblum reviewed Aug 19, 2016
View reviewed changes

victorhurdugaci reviewed Aug 25, 2016
View reviewed changes

pakrym force-pushed the pakrym/blob branch 3 times, most recently from 6dfa193 to dad79f0 Compare August 25, 2016 21:43

pakrym added 2 commits August 25, 2016 14:44

Add Blob AzureWebApp provider

a3ac1f3

Adress PR comments

248ae2b

pakrym force-pushed the pakrym/blob branch from dad79f0 to 248ae2b Compare August 25, 2016 21:45

victorhurdugaci reviewed Aug 29, 2016
View reviewed changes

pakrym force-pushed the pakrym/blob branch 2 times, most recently from 75f3da0 to 223cae6 Compare August 29, 2016 19:12

pakrym force-pushed the pakrym/blob branch from 223cae6 to b11a8f7 Compare August 29, 2016 19:36

victorhurdugaci reviewed Aug 29, 2016
View reviewed changes

pakrym force-pushed the pakrym/blob branch from b11a8f7 to fedb9b0 Compare August 29, 2016 22:13

Add tests

862a2df

pakrym force-pushed the pakrym/blob branch from fedb9b0 to 862a2df Compare August 29, 2016 22:25

pakrym merged commit 88ee78a into dev Aug 30, 2016

pakrym deleted the pakrym/blob branch October 4, 2016 16:13

This was referenced Oct 5, 2016

Don't subclass SerilogLoggerProvider #471

Closed

Make the WebApps logger configurable #470

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Blob AzureWebApp provider #477

Add Blob AzureWebApp provider #477

pakrym commented Aug 16, 2016

pakrym commented Aug 17, 2016

davidfowl Aug 17, 2016

campersau Aug 17, 2016

victorhurdugaci commented Aug 19, 2016

pakrym commented Aug 19, 2016

cesarblum Aug 19, 2016

victorhurdugaci Aug 25, 2016

victorhurdugaci commented Aug 25, 2016

pakrym commented Aug 25, 2016

victorhurdugaci commented Aug 26, 2016

pakrym commented Aug 29, 2016

victorhurdugaci Aug 29, 2016

victorhurdugaci Aug 29, 2016

pakrym commented Aug 29, 2016

victorhurdugaci Aug 29, 2016

pakrym Aug 29, 2016

victorhurdugaci commented Aug 29, 2016

pakrym commented Aug 30, 2016

Add Blob AzureWebApp provider #477

Add Blob AzureWebApp provider #477

Conversation

pakrym commented Aug 16, 2016

pakrym commented Aug 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorhurdugaci commented Aug 19, 2016

pakrym commented Aug 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorhurdugaci commented Aug 25, 2016

pakrym commented Aug 25, 2016

victorhurdugaci commented Aug 26, 2016

pakrym commented Aug 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakrym commented Aug 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorhurdugaci commented Aug 29, 2016

pakrym commented Aug 30, 2016