Updates to `feature/egressExtension` Part 1 #2748

kkeirstead · 2022-10-20T19:16:49Z

Summary

Part 1 of my updates to the feature/egressExtension branch - I pulled out some things to keep this PR smaller, as well as areas that I'm still working through. I'll leave comments throughout the code to explain different things that I think are worth pointing out or that I've cut out (for now). Builds on top of #1953.

Release Notes Entry

… addition to supporting writing configuration in the main location

…extensibility/egress-extensibility.md

… - this did require (for now) some duplication of files

…d wiring up the fourth location (.dotnet\tools) to check for extensions

…ol install to be the landing site for third party extensions - needs refinement (currently uses some hard-coded values)

…; have not tested these changes

…ettle on a better mechanism for passing non string:string configuration

kkeirstead · 2022-10-20T19:18:20Z

src/Extensions/AzureBlobStorage.sln

@@ -0,0 +1,25 @@
+
+Microsoft Visual Studio Solution File, Format Version 12.00


We aren't doing it yet, but in the future we'll be moving this to the new extensions repo

kkeirstead · 2022-10-20T19:19:19Z

src/Extensions/AzureBlobStorage/EgressArtifactSettings.cs

+        /// <summary>
+        /// Custom metadata of the blob to be created.
+        /// </summary>
+        public Dictionary<string, string> CustomMetadata { get; }


Not currently being populated (I'll mention more about this in another comment)

kkeirstead · 2022-10-20T19:23:03Z

src/Microsoft.Diagnostics.Monitoring.Options/ExtensionEgressProviderOptions.cs


 namespace Microsoft.Diagnostics.Tools.Monitor.Egress
 {
    /// <summary>
    /// Egress provider options for external egress providers.
    /// </summary>
    internal sealed partial class ExtensionEgressProviderOptions : 
-        Dictionary<string, string>,
+        Dictionary<string, object>,


This isn't implemented in this PR, but in the initial iteration of this design we were assuming that we could treat all of our extension's configuration as a Dictionary<string, string>. The addition of the Azure provider's Metadata (used for custom metadata) breaks this assumption, and will require changes to how we pass the configuration to the extension. To simplify this PR, I am not currently passing the Metadata section of configuration, but this will be fixed in a future PR.

kkeirstead · 2022-10-20T19:24:29Z

...osoft.Diagnostics.Monitoring.Tool.UnitTests/Egress/AzureBlob/AzureBlobEgressProviderTests.cs

@@ -12,7 +12,7 @@
 using Microsoft.Diagnostics.Monitoring.TestCommon.Fixtures;
 using Microsoft.Diagnostics.Monitoring.WebApi;
 using Microsoft.Diagnostics.Tools.Monitor.Egress;
-using Microsoft.Diagnostics.Tools.Monitor.Egress.AzureBlob;
+//using Microsoft.Diagnostics.Tools.Monitor.Egress.AzureBlob;


(Relevant to the whole file) For the sake of simplicity, the tests for Azure egress aren't enabled in this PR. These will be re-integrated in a future PR, as well as hopefully more testing specifically around the end-to-end extension experience.

kkeirstead · 2022-10-20T19:25:23Z

src/Tools/dotnet-monitor/Egress/Configuration/EgressProviderConfigureNamedOptions.cs

@@ -34,7 +34,23 @@ public void Configure(string name, TOptions options)
                IConfigurationSection providerOptionsSection = providerTypeSection.GetSection(name);
                if (providerOptionsSection.Exists())
                {
-                    providerOptionsSection.Bind(options);
+                    if (options is ExtensionEgressProviderOptions extensionOptions)


This is where we'll need to handle Metadata (and anything else that doesn't have a value for a child).

kkeirstead · 2022-10-20T19:27:53Z

src/Tools/dotnet-monitor/Extensibility/FolderExtensionRepository.cs

+
+            bool isDotnetToolsLocation = _targetFolder == HostBuilderSettings.ExtensionDirectoryPath;
+
+            // Special case -> need to look in particular path for dotnet tool installation


This logic is for the dotnet tool install scenario, so that users don't need to manually place an extension downloaded from Nuget into the right directory. This works in my limited testing (building a nuget package locally, and then calling dotnet tool install --add-source... on it), but I think this area may need some more refinement going forward.

kkeirstead · 2022-10-20T19:29:16Z

src/Tools/dotnet-monitor/LoggingEventIds.cs

-        QueueDoesNotExist = 57,
-        QueueOptionsPartiallySet = 58,
-        WritingMessageToQueueFailed = 59,
+        MonitorApiKeyNotConfigured = 56, // Note the gap - from removing things related to Azure egress


Will we want to update all of these numbers to be in-order again? I haven't for now, but we have several gaps left behind by the logging specific to the Azure extension.

Do not remove or reorder anything here. Just add a comment after then that they've moved to the extension.

schmittjoseph · 2022-10-20T21:10:14Z

src/Tools/dotnet-monitor/Extensibility/FolderExtensionRepository.cs

-            IDirectoryContents extensionDir = _fileSystem.GetDirectoryContents(extensionName);
+            string extensionPath = string.Empty;
+
+            bool isDotnetToolsLocation = _targetFolder == HostBuilderSettings.ExtensionDirectoryPath;


If you aren't guaranteed that _targetFolder is an absolute path, you'll want to resolve it.

If on Windows, paths are case insensitive.

Suggested change

bool isDotnetToolsLocation = _targetFolder == HostBuilderSettings.ExtensionDirectoryPath;

bool isDotnetToolsLocations = string.Equals(

Path.GetFullPath(_targetFolder),

Path.GetFullPath(HostBuilderSettings.ExtensionDirectoryPath),

RuntimeInformation.IsOSPlatform(OSPlatform.Windows)

? StringComparison.OrdinalIgnoreCase

: StringComparison.Ordinal);

Consider adding a helper method such as ArePathsEqual to help simplify this suggested code.

schmittjoseph · 2022-10-20T21:13:38Z

src/Extensions/AzureBlobStorage/AzureBlobStorage.csproj

+    <!--
+      Workaround for https://github.com/dotnet/aspnetcore/issues/42200
+      Required as of 7.0 Preview 5 SDK
+      -->
+    <StaticWebAssetsEnabled>false</StaticWebAssetsEnabled>


Suggested change



<StaticWebAssetsEnabled>false</StaticWebAssetsEnabled>

I think this should be reverted, see #2700

schmittjoseph · 2022-10-20T21:18:48Z

src/Extensions/AzureBlobStorage/Program.cs

            if (configDict.ContainsKey(propKey))
            {
-                return configDict[propKey];
+                return configDict[propKey].ToString();
            }
            return null;


Suggested change

if (configDict.ContainsKey(propKey))

{

return configDict[propKey];

return configDict[propKey].ToString();

}

return null;

if (configDict.TryGetValue(propKey, out object propValue))

{

return propValue.ToString();

}

return null;

suggestion: can update this to only access the dictionary once

jander-msft · 2022-10-21T20:39:51Z

src/Extensions/AzureBlobStorage.sln

@@ -0,0 +1,25 @@
+


Remove this and add the AzureBlobStorage project to the dotnet-monitor.sln file if it already doesn't exist.

jander-msft · 2022-10-21T20:52:16Z

src/Extensions/AzureBlobStorage/AzureBlob/AzureBlobEgressProvider.cs

    {
-        private int BlobStorageBufferSize = 4 * 1024 * 1024;
+        private readonly string AzureBlobStorage = "AzureBlobStorage";


This should be moved to some kind of shared constant because it is used in the same way both here and in the Program class.

jander-msft · 2022-10-21T20:54:48Z

src/Extensions/AzureBlobStorage/AzureBlob/AzureBlobEgressProvider.cs

    {
-        private int BlobStorageBufferSize = 4 * 1024 * 1024;
+        private readonly string AzureBlobStorage = "AzureBlobStorage";
+        protected ILogger Logger { get; }


This should be a private field.

jander-msft · 2022-10-21T20:55:57Z

src/Extensions/AzureBlobStorage/Program.cs

        static async Task<int> Main(string[] args)
        {
-            // Expected command line format is: AzureBlobStorage.exe Egress --Provider-Name MyProviderEndpointName
-            RootCommand rootCommand = new RootCommand("Egresses an artifact to Azure Storage.");
+            using var loggerFactory = LoggerFactory.Create(builder =>


This should be done in the Egress method and not stored as static state on the Program class.

jander-msft · 2022-10-21T20:57:53Z

src/Extensions/CODE-OF-CONDUCT.md

@@ -0,0 +1,6 @@
+# Code of Conduct


Do not create a separate file here. This is still part of the dotnet-monitor repo so it should be using the files at the root.

jander-msft · 2022-10-21T20:58:10Z

src/Extensions/LICENSE.TXT

@@ -0,0 +1,23 @@
+The MIT License (MIT)


Do not create a separate license file here. This is still part of the dotnet-monitor repo so it should be using the files at the root.

kkeirstead · 2022-10-21T21:01:12Z

Closing this PR in favor of breaking things down even further for ease of review. I will incorporate the feedback already left on this PR into my changes.

jander-msft · 2022-10-21T21:03:17Z

src/Tools/dotnet-monitor/Egress/Configuration/EgressProviderConfigureNamedOptions.cs

-                    providerOptionsSection.Bind(options);
+                    if (options is ExtensionEgressProviderOptions extensionOptions)
+                    {
+                        var children = providerOptionsSection.GetChildren();


Why is this needed? Are you working around this issue? dotnet/runtime#77246

jander-msft · 2022-10-21T21:04:18Z

src/Tools/dotnet-monitor/Egress/Extension/ExtensionEgressProvider.cs

@@ -43,8 +42,7 @@ public ExtensionEgressProvider(IEgressPropertiesProvider propertyProvider, Exten
            {
                Settings = artifactSettings,
                Configuration = options,
-                Properties = _propertyProvider.GetAllProperties(),
-                ProviderName = providerName,


Why is the provider name being removed?

jander-msft · 2022-10-21T21:12:49Z

src/Tools/dotnet-monitor/HostBuilder/HostBuilderSettings.cs

@@ -42,6 +44,16 @@ internal sealed class HostBuilderSettings
                    Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), "." + ProductFolderName) :
                    Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData), ProductFolderName));

+        // Location where extensions are stored by default.
+        // Windows: "%USERPROFILE%\.dotnet\Tools"
+        // Other: "%XDG_CONFIG_HOME%/dotnet/tools" OR "%HOME%/.config/dotnet/tools" -> IS THIS RIGHT?


It appears to be the same as on Windows:

jander@DESKTOP-N8NH8I6:~/.dotnet/tools$ ls dotnet-dump dotnet-monitor dotnet-sos dotnet-trace

jander-msft · 2022-10-21T21:15:07Z

src/Tools/dotnet-monitor/Extensibility/FolderExtensionRepository.cs

-            IDirectoryContents extensionDir = _fileSystem.GetDirectoryContents(extensionName);
+            string extensionPath = string.Empty;
+
+            bool isDotnetToolsLocation = _targetFolder == HostBuilderSettings.ExtensionDirectoryPath;


I think you should have a separate ExtensionRepository implementation for the dotnet tools folder since its algorithm is very much different than the FolderExtensionRepository implementation.

kkeirstead and others added 30 commits October 18, 2022 11:50

Typo in comments

604cecf

Broken; just doing some experimenting

757b878

Now able to pull in extensions JSON files from expected locations, in…

419eb63

… addition to supporting writing configuration in the main location

Create egress-extensibility.md

66dc367

Update egress-extensibility.md

3820f0b

Update egress-extensibility.md

581a517

Create template.md

8815f2a

Rename documentation/egress-extensibility.md to documentation/egress-…

e750d13

…extensibility/egress-extensibility.md

Update template.md

ab949f8

Update template.md

0007678

Update template.md

fd43014

Completely broke off Azure Blob Storage extension from dotnet monitor…

b75675c

… - this did require (for now) some duplication of files

Minor changes

69d2db0

Swapped logging system to Console output for AzureBlobStorage; starte…

9781efe

…d wiring up the fourth location (.dotnet\tools) to check for extensions

Made the AzureBlobStorage csproj packable

ef898cd

Don't generate documentation

ec05a23

Added tool command name

198c44c

Experimenting

7a639cf

AzureBlobStorage is now buildable by dotnet tool

3ad0085

Added in pathways for special-case extension installed by dotnet tool

a863b3d

Wired through scenario that allows the default location for dotnet to…

62dac41

…ol install to be the landing site for third party extensions - needs refinement (currently uses some hard-coded values)

Small tweaks

b6e7ad5

Pulled out provider name

13f2453

Split unit tests out to be part of a solution with the azure provider…

0b0b5e9

…; have not tested these changes

Fixes to get unit tests passing

5ed4a85

Removing some unused files; still works

78e4977

Update egress-extensibility.md

0c9c4a8

Create contributors.md

df9d4e9

Rename contributors.md to contributing-guidelines.md

2759067

Create how-to-use.md

55edfe0

kkeirstead added 5 commits October 20, 2022 07:07

Updated schema, removed passing metadata (custom) for now until can s…

5527c81

…ettle on a better mechanism for passing non string:string configuration

More tweaks for PR

ee05d54

Bug fixes

9da2d8b

Schema update

da0718a

Pulled out some commented code

e78601c

kkeirstead requested a review from a team as a code owner October 20, 2022 19:16

kkeirstead commented Oct 20, 2022

View reviewed changes

schmittjoseph reviewed Oct 20, 2022

View reviewed changes

jander-msft reviewed Oct 21, 2022

View reviewed changes

kkeirstead closed this Oct 21, 2022

jander-msft reviewed Oct 21, 2022

View reviewed changes

This was referenced Oct 22, 2022

Updates to feature/egressExtension Part 1 #2759

Merged

Updates to feature/egressExtension Part 2 #2790

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates to `feature/egressExtension` Part 1 #2748

Updates to `feature/egressExtension` Part 1 #2748

kkeirstead commented Oct 20, 2022

kkeirstead Oct 20, 2022

kkeirstead Oct 20, 2022

kkeirstead Oct 20, 2022

kkeirstead Oct 20, 2022

kkeirstead Oct 20, 2022

kkeirstead Oct 20, 2022

kkeirstead Oct 20, 2022

jander-msft Oct 21, 2022

schmittjoseph Oct 20, 2022

schmittjoseph Oct 20, 2022

schmittjoseph Oct 20, 2022 •

edited

Loading

jander-msft Oct 21, 2022

jander-msft Oct 21, 2022

jander-msft Oct 21, 2022

jander-msft Oct 21, 2022

jander-msft Oct 21, 2022

jander-msft Oct 21, 2022

kkeirstead commented Oct 21, 2022

jander-msft Oct 21, 2022

jander-msft Oct 21, 2022

jander-msft Oct 21, 2022

jander-msft Oct 21, 2022

		@@ -0,0 +1,25 @@

		Microsoft Visual Studio Solution File, Format Version 12.00


		bool isDotnetToolsLocation = _targetFolder == HostBuilderSettings.ExtensionDirectoryPath;

		// Special case -> need to look in particular path for dotnet tool installation

-            bool isDotnetToolsLocation = _targetFolder == HostBuilderSettings.ExtensionDirectoryPath;
+    bool isDotnetToolsLocations = string.Equals(
+        Path.GetFullPath(_targetFolder),
+        Path.GetFullPath(HostBuilderSettings.ExtensionDirectoryPath),
+        RuntimeInformation.IsOSPlatform(OSPlatform.Windows)
+            ? StringComparison.OrdinalIgnoreCase
+            : StringComparison.Ordinal);

Updates to feature/egressExtension Part 1 #2748

Updates to feature/egressExtension Part 1 #2748

Conversation

kkeirstead commented Oct 20, 2022

Summary

Release Notes Entry

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmittjoseph Oct 20, 2022 • edited Loading

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

kkeirstead commented Oct 21, 2022

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

Updates to `feature/egressExtension` Part 1 #2748

Updates to `feature/egressExtension` Part 1 #2748

schmittjoseph Oct 20, 2022 •

edited

Loading