Azure blob in flat folder structure #25078

yogigrantz · 2020-08-20T17:10:42Z

Added an option to output Azure blob in flat folder structure with custom filename prefix

Summary of the changes (Less than 80 chars)

Added internal static string to store the custom prefix
Added public overload to include the custom name parm
Modified existing nested folder creation to accept the flat folder option

Addresses #25885

previous discussion: dotnet/extensions#3391

…stom filename prefix

wtgodbe · 2020-08-20T20:46:12Z

@JunTaoLuo who owns this package now, since it's moved from Extensions?

JunTaoLuo · 2020-08-21T21:05:56Z

Good question for @Pilchie

Pilchie · 2020-08-24T18:16:30Z

@davidfowl do you know who knows about this package?

shirhatti · 2020-08-24T18:57:10Z

Pavel created the package and no one has touched it since.

Pilchie · 2020-08-25T21:25:43Z

Also tagging @javiercn.

davidfowl · 2020-08-25T21:44:16Z

src/Logging.AzureAppServices/src/BlobLoggerProvider.cs

            foreach (var eventGroup in eventGroups)
            {
                var key = eventGroup.Key;
-                var blobName = $"{_appName}/{key.Year}/{key.Month:00}/{key.Day:00}/{key.Hour:00}/{_fileName}";
+                //var blobName = $"{_appName}/{key.Year}/{key.Month:00}/{key.Day:00}/{key.Hour:00}/{_fileName}";


Why is this commented out?

That code was replaced with the logic from line 66 - 74 because it forces nested folder in Azure blob. Line 71 produces a flat folder structure, while line 74 produces the original nested folder structure where the code was commented out.

Shouldn't it be removed then? Why did you leave the code in but commented out?

Yes you're right, it should be removed. I will remove it and commit my change

@davidfowl : I removed the commented out code, committed and pushed. This is my first time contributing, and I was not sure whether to leave commented out original code or to delete. But now I know to go ahead and delete when deemed not needed. Thank you.

@davidfowl : After removing the commented our code, it would not pass the checks. Could you please advise what might have gone wrong, could this be something on the CI side?

@davidfowl : The build is ready for your review. Could you please revisit this PR? Thank you!

wtgodbe · 2020-08-26T23:08:14Z

We were having some issues with our selenium tests, they should be fixed now. This should pass on retry

wtgodbe · 2020-08-26T23:08:18Z

/azp run

azure-pipelines · 2020-08-26T23:08:38Z

Azure Pipelines successfully started running 2 pipeline(s).

yogigrantz · 2020-08-27T04:22:06Z

Azure Pipelines successfully started running 2 pipeline(s).

still failing? hmmm.... could we try again please?

wtgodbe · 2020-08-27T14:35:52Z

/azp run

azure-pipelines · 2020-08-27T14:36:09Z

Azure Pipelines successfully started running 2 pipeline(s).

wtgodbe · 2020-08-27T15:32:34Z

@yogigrantz the CI failures are unrelated, we're having some issues w/ our template tests at the moment. Should hopefully be resolved soon.

yogigrantz · 2020-08-27T17:30:17Z

@yogigrantz the CI failures are unrelated, we're having some issues w/ our template tests at the moment. Should hopefully be resolved soon.

Thank you William!

wtgodbe · 2020-08-27T22:28:00Z

/azp run

azure-pipelines · 2020-08-27T22:28:16Z

Azure Pipelines successfully started running 2 pipeline(s).

yogigrantz · 2020-08-28T23:51:01Z

/azp run

still failing ... I should have deleted that line in the first place :(

Pilchie · 2020-08-29T00:13:11Z

/azp run

Pilchie · 2020-08-29T00:13:21Z

Let's give it another shot.

azure-pipelines · 2020-08-29T00:13:27Z

Azure Pipelines successfully started running 2 pipeline(s).

Pilchie · 2020-08-29T23:07:43Z

@davidfowl or @javiercn - can one of you review this?

yogigrantz · 2020-08-30T03:50:09Z

Let's give it another shot.

All checks have passed now :) Thank you All!

yogigrantz · 2020-09-22T16:21:15Z

@davidfowl @javiercn @Pilchie @wtgodbe @JunTaoLuo @shirhatti
Could I please have this pull request approved before the CI automatically closes this issue. This feature is very important to our company to avoid workaround custom/duplicate Extension API.

I can put sample Azure application in GitHub if that helps?. Please do let me know. Thank you so much, appreciate your time!

dougbu · 2020-10-15T22:10:32Z

@dougbu could you remind me how to do that?

I strongly recommend not trying to change the PublicAPI.Unshipped.txt files manually. The analyzer has fairly nice fixers that work only in Visual Studio. So,

Open a (preferably small) *.slnf file containing the project with API changes in VS
Build the solution
Click on one of the errors
Use the light bulb 💡 or right click to fix the error globally. I say globally because it's a pain to click on every error and fix it individually
For errors at a lower level than added, changed, or removed surface area, you may need to either fix the API or suppress the error e.g. https://github.com/dotnet/aspnetcore/blob/master/src/Components/Components/src/EventCallbackFactoryBinderExtensions.cs#L35

dougbu · 2020-10-15T22:11:38Z

Oh, it may be necessary to repeat the above in VS because the first round didn't get far enough to hit all of the public API issues

davidfowl · 2020-10-16T10:06:48Z

src/Logging.AzureAppServices/src/AzureAppServicesLoggerFactoryExtensions.cs

-        public static ILoggingBuilder AddAzureWebAppDiagnostics(this ILoggingBuilder builder)
+        /// <param name="customPrefix"></param>
+        /// <returns></returns>
+        public static ILoggingBuilder AddAzureWebAppDiagnostics(this ILoggingBuilder builder, string customPrefix = null)


@Pilchie is this a binary breaking change?

Absolutely.

Overloads are binary breaking safe, optional parameters are not - think about how they work, the method always receives two arguments, it's just that if you don't specify it, the compiler burns in the default value at the call-site.

My bad, I removed the optional parm and went ahead and created an overload instead. But CI still shows this optional parm error, even though I renamed the parameter.
@Pilchie @davidfowl
[SuppressMessage("ApiDesign", "RS0026:Do not add multiple public overloads with optional parameters"

Removed the optional parameter, created overload to handle the extra parameter

Gihub repository did not take my change for removig the optional = null, so I am renaming the parm

yogigrantz · 2020-10-19T23:27:40Z

Please discard my version of AspNetCore.sln. It was inadvertently pushed to my repository

yogigrantz · 2020-10-20T22:24:47Z

Could someone please help me out? I accidentally pushed AspnetCore.sln. Now it is in conflict. I'd appreciate if someone with write access can resolve conflicts by replacing my version with the current master version. Thank you!

BrennanConroy · 2020-10-20T22:28:09Z

You should be able to fix it fairly easily:

git revert --hard HEAD~2
# redo the parameter naming change
git add <file>
git commit -m "param rename"
git push -f

yogigrantz · 2020-10-21T01:10:09Z

@BrennanConroy when I typed git revert --hard HEAD~2 it would not take. it gave me the help menu, shown in this screenshot:

BrennanConroy · 2020-10-21T01:14:40Z

Whoops, I meant reset. git reset --hard HEAD~2

yogigrantz · 2020-10-21T02:10:11Z

Brennan: I followed your steps. Not sure if the .sln file was recovered? But could I have another rebuild please? I think the last build broke with #L39

Renamed the parm back. Hoping to get a clean build

yogigrantz · 2020-10-21T03:41:55Z

@wtgodbe : is there anything I could do to fix this error ?src/Logging.AzureAppServices/src/AzureAppServicesLoggerFactoryExtensions.cs(39,39): error RS0016: (NETCORE_ENGINEERING_TELEMETRY=Build) Symbol 'AddAzureWebAppDiagnostics' is not part of the declared API.

dougbu · 2020-10-21T04:40:14Z

is there anything I could do to fix this error ?

If you have Visual Studio 16.8 or later installed,

.\restore.cmd to ensure the .dotnet/ folder is up-to-date
.\startvs.cmd src\Logging.AzureAppServices\src\Microsoft.Extensions.Logging.AzureAppServices.csproj
build the project e.g. [F6]
click on the RS0016 message in the error window
use the light bulb menu or right click the line and add the method to the public API files
commit the change and update this branch 😺

yogigrantz · 2020-10-21T16:54:00Z

Would this work in VS 16.7.6? because when I checked my VS it says 16.7.6 and that's the latest update.

When I followed your steps I did not get any errors. I took the screenshots below. I'm installing the 16.8 preview 3.1 version now ...

With VS 16.8 Preview 5.0 (from Visual Studio):

dougbu · 2020-10-21T17:17:16Z

Would this work in VS 16.7.6?

@mavasani @sharwell do the public API analyzers and fixers work correctly in 16.7.6❔ I'm surprised @yogigrantz's build was successful.

@yogigrantz it's possible VS thought the project was up-to-date. You may need to rebuild instead of build to get the error seen on the CI.

Another idea would be to use the public preview version of Visual Studio. Easy way to get that is .\eng\scripts\InstallVisualStudio.ps1 -Channel Preview -Edition Community.

yogigrantz · 2020-10-21T18:58:49Z

@dougbu : The rebuild works both in 16.7.6 and 16.8 Preview 5.0. When I downloaded preview 3.1 it gave me version 5.0 btw so I could not get 3.1.

The .\restore.cmd failed on 16.8 preview 5.0.

Also, when I ran this script: .\eng\scripts\InstallVisualStudio.ps1 -Channel Preview -Edition Community it was pointing to version 16.7.6

dougbu · 2020-10-21T19:30:24Z

The rebuild works both in 16.7.6 and 16.8 Preview 5.0.

I don't understand this and hope @mavasani or @sharwell can help out here.

When I downloaded preview 3.1 it gave me version 5.0 btw so I could not get 3.1

I didn't mention a specific preview of VS 16.8. Preview 5 is fine.

it was pointing to version 16.7.6

What pointed to version 16.7.6❔ If you mean the version of VS that startvs.cmd chose to start, That would mean the default is still your previous VS installation.

You can change the association for *.csproj files to use the Preview version of VS by default. Or, you can set $env:VSINSTALLDIR to C:\Program Files (x86)\Microsoft Visual Studio\2019\Community_Pre" before using .\startvs.cmd. The InstallVisualStudio.ps1 command I provided should have installed VS 16.8 there.

mavasani · 2020-10-21T20:37:56Z

@dougbu @yogigrantz You are likely hitting dotnet/msbuild#3046, which is applicable to every MSBuild warning, not just analyzer warnings.

yogigrantz · 2020-10-21T21:50:11Z

@dougbu : What pointed to version 16.7.6❔ Sorry for the confusion. when I copied your command line, it errored out. So I went ahead and opened InstallVisualStudio.ps1 with powershell and ran it manually. At that point it opened up the Installer option dialog for Enterprise edition, 16.7.6 . After that I changed the value for $Edition and $Channel as shown on the screenshot, but all it does is open the installer dialog with items that I already installed checked out. Is this step for verifying whether I have the correct Visual Studio?

yogigrantz · 2020-10-21T21:59:17Z

@mavasani : In our case we don't have any warning though. I build with --no-incremental and still, no warning

dougbu · 2020-10-22T01:49:15Z

@yogigrantz for the questions you asked me: The InstallVisualStudio.ps1 file is intended to run from within PowerShell and we don't have a .bat or .cmd file anywhere to make it easier to use from cmd. When run without -quiet or -passive, the script does indeed open the installer dialogue. You need to click Install, Modify, or Update to complete the installation or upgrade.

yogigrantz · 2020-10-23T06:05:59Z

@dougbu ok I followed thru with update and everything went well. I did another build, it built successfully. 0 Warning 0 Error. There is no changed file in my repository, so there is nothing to commit or push. @mavasani : when there is no warning and no error, does this situation in dotnet/msbuild#304 still apply? Could you please advise how I should resolve it?

yogigrantz · 2020-10-30T18:22:17Z

Hi, since my repository was too far behind I deleted it and reforked and recloned aspnetcore from the master branch. The current global.json says sdk "version": "6.0.100-alpha.1.20523.3". Should I download this sdk to build the solution, because I have 5.0-rc2 in my machine and it would not build this solution now. If I modify global.json chances are, when I try to merge it would fail the build again. Also I did not see SDK 6.0 being available to download. Please advise and give the link if I should use 6.0, thank you!
@dougbu @mavasani @wtgodbe

dougbu · 2020-10-30T19:34:10Z

Should I download this sdk to build the solution, because I have 5.0-rc2 in my machine and it would not build this solution now.

Run the ./build.cmd script w/ pretty much any option to install the required SDK and runtimes in the .dotnet/ folder. The startvs.cmd ensures Visual Studio uses that folder and not the globally-installed dotnet. All command-line builds also ignore the globally-installed dotnet.

yogigrantz · 2020-10-31T00:40:21Z

Started over from fresh copy of dotnetcore master branch 10/3/2020 -> pull request #27383. I updated / fixed the UnitTest and added the new method definition in src/Logging.AzureAppServices/src/PublicAPI.Unshipped.txt . Now getting only 3 failures
. Could you please let me know whether this failure come from the CI or from the application? Thank you!
@dougbu @wtgodbe

dougbu · 2020-10-31T00:58:33Z

@yogigrantz you need to use Visual Studio to update the src/Logging.AzureAppServices/src/PublicAPI.Unshipped.txt file. It should include the added AddAzureWebAppDiagnostics overload.

You may need to remove the .\artifacts\bin\Microsoft.Extensions.Logging.AzureAppServices\ folder and perhaps .\artifacts\obj\Microsoft.Extensions.Logging.AzureAppServices\ to ensure the VS build actually hits the error.

yogigrantz · 2020-11-02T17:41:32Z

Transferring to #27383 -> Fresh reclone

Added an option to output Azure blob in flat folder structure with cu…

d3110c0

…stom filename prefix

yogigrantz mentioned this pull request Aug 20, 2020

Optional Flat Blob file structure and custom prefix dotnet/extensions#3391

Closed

BrennanConroy added the feature-azure Issues related to integration with Azure components (Azure App Service Logging, App Insights, etc.) label Aug 21, 2020

Pilchie assigned davidfowl Aug 24, 2020

davidfowl reviewed Aug 25, 2020

View reviewed changes

Remove the commented code - not needed anymore

e89228d

Pilchie added the community-contribution Indicates that the PR has been added by a community member label Aug 27, 2020

BrennanConroy added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Sep 14, 2020

davidfowl reviewed Oct 16, 2020

View reviewed changes

YogiGrantz added 2 commits October 16, 2020 11:02

Fixed binary breaking change

917a1d3

Removed the optional parameter, created overload to handle the extra parameter

Renamed parm

88eb54f

Gihub repository did not take my change for removig the optional = null, so I am renaming the parm

Renamed the parm back

2adb379

Renamed the parm back. Hoping to get a clean build

yogigrantz mentioned this pull request Oct 30, 2020

Added overload to allow using custom prefix name and flat folder stru… #27383

Closed

yogigrantz closed this Nov 2, 2020

Azure blob in flat folder structure #25078

Azure blob in flat folder structure #25078

Conversation

yogigrantz commented Aug 20, 2020 • edited by BrennanConroy Loading

wtgodbe commented Aug 20, 2020

JunTaoLuo commented Aug 21, 2020

Pilchie commented Aug 24, 2020

shirhatti commented Aug 24, 2020

Pilchie commented Aug 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtgodbe commented Aug 26, 2020

wtgodbe commented Aug 26, 2020

azure-pipelines bot commented Aug 26, 2020

yogigrantz commented Aug 27, 2020

wtgodbe commented Aug 27, 2020

azure-pipelines bot commented Aug 27, 2020

wtgodbe commented Aug 27, 2020

yogigrantz commented Aug 27, 2020

wtgodbe commented Aug 27, 2020

azure-pipelines bot commented Aug 27, 2020

yogigrantz commented Aug 28, 2020

Pilchie commented Aug 29, 2020

Pilchie commented Aug 29, 2020

azure-pipelines bot commented Aug 29, 2020

Pilchie commented Aug 29, 2020

yogigrantz commented Aug 30, 2020

yogigrantz commented Sep 22, 2020

dougbu commented Oct 15, 2020

dougbu commented Oct 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yogigrantz commented Oct 19, 2020

yogigrantz commented Oct 20, 2020

BrennanConroy commented Oct 20, 2020

yogigrantz commented Oct 21, 2020

BrennanConroy commented Oct 21, 2020

yogigrantz commented Oct 21, 2020

yogigrantz commented Oct 21, 2020

dougbu commented Oct 21, 2020 • edited Loading

yogigrantz commented Oct 21, 2020 • edited Loading

dougbu commented Oct 21, 2020

yogigrantz commented Oct 21, 2020

dougbu commented Oct 21, 2020

mavasani commented Oct 21, 2020

yogigrantz commented Oct 21, 2020

yogigrantz commented Oct 21, 2020

dougbu commented Oct 22, 2020

yogigrantz commented Oct 23, 2020

yogigrantz commented Oct 30, 2020 • edited Loading

dougbu commented Oct 30, 2020

yogigrantz commented Oct 31, 2020 • edited Loading

dougbu commented Oct 31, 2020

yogigrantz commented Nov 2, 2020

yogigrantz commented Aug 20, 2020 •

edited by BrennanConroy

Loading

dougbu commented Oct 21, 2020 •

edited

Loading

yogigrantz commented Oct 21, 2020 •

edited

Loading

yogigrantz commented Oct 30, 2020 •

edited

Loading

yogigrantz commented Oct 31, 2020 •

edited

Loading