New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Trace notifications #2604

Merged

michaelcfanning merged 10 commits into main from trace-notifications

Jan 13, 2023

Member

michaelcfanning commented Jan 11, 2023

No description provided.

michaelcfanning added 8 commits

January 5, 2023 15:37


          Add new rule timing and general execution trace.

f47aa0f


          Add new rule timing and general execution trace.

7562efd


          Merge branch 'main' into new-traces

2ac8e11


          Eliminate redundant entries from bad merge in release notes.

4bfc4c5


          Tests and improvements for scan time notifications.

9c746cb


          Update trace notifications capability.

c947ca0


          Merge branch 'main' into trace-notifications

b55408d


          Merge branch 'trace-notifications' of https://github.com/microsoft/sa…

1ec3e4e

…rif-sdk into trace-notifications

michaelcfanning commented

View reviewed changes

src/Sarif/IAnalysisLogger.cs

+                      /// <param name="associatedRule">
+                      /// The scan rule, if any, associated with the notification.
+                      /// </param>
+                      void LogToolNotification(Notification notification, ReportingDescriptor associatedRule = null);

Member Author

michaelcfanning Jan 12, 2023 •

edited

Loading

associatedRule = null

An optional argument in an interface contract is an interesting thing. Any caller to the interface does not need to provide this argument. Anyone implementing this interface can override the optional value.

That can get confusing. I made this change to minimize code churn, so that callers who don't have an associated rule aren't strictly required to provide one.

michaelcfanning commented

View reviewed changes

src/Sarif.Driver/Sdk/AnalyzeCommandBase.cs

                               Policy = policy ?? new PropertiesDictionary()
                           };
+                          context.Traces =

Member Author

michaelcfanning Jan 12, 2023 •

edited

Loading

context

This handling can now be removed from upstream driver tools that are currently required to do the same thing.

In point of fact, I didn't need to add this change as we're not keeping analyze command up to date with all features, it will get removed shortly.


          Restore tests to working order.

michaelcfanning marked this pull request as ready for review

January 12, 2023 17:53

michaelcfanning requested review from EasyRhinoMSFT, marmegh and cfaucon as code owners

January 12, 2023 17:53

michaelcfanning commented

View reviewed changes

src/Sarif/Writers/SarifLogger.cs

@@ @@ -479,6 +479,16 @@ public void LogToolNotification(Notification notification) @@
                           _run.Invocations[0].ExecutionSuccessful &= notification.Level != FailureLevel.Error;
                           CaptureFilesInNotification(notification);
+                          if (associatedRule != null)

Member Author

michaelcfanning Jan 12, 2023

associatedRule

With this change, the rule metadata for any notification associated with a rule will be persisted to the rules table in the log.

michaelcfanning commented

View reviewed changes

src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs

                               try
                               {
+                                  Stopwatch stopwatch = context.Traces.HasFlag(DefaultTraces.RuleScanTime)

Member Author

michaelcfanning Jan 12, 2023

stopwatch

Shaopeng, here's a clean pattern for producing timing data on a per-rule + target file basis. We can also consider adding memory consumption information in a similar change, if helpful.

michaelcfanning commented

View reviewed changes

src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs

                           return run;
                       }
+                      [Fact]
+                      public void AnalyzeCommand_Traces()

Member Author

michaelcfanning Jan 12, 2023

AnalyzeCommand_Traces

Good things about this test: there are minimal literal constant 'magic values', it runs a comprehensive set of tests and will report on all break (instead of failing on the first one), it nicely buckets all trace testing in one place and can easily be extended. This change also makes some nice incremental improvements to the general test methodology, for example, returning the Run object from the test helper to assist in validation.

michaelcfanning commented

View reviewed changes

src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs

+                              int executionNotificationsCount = 0;
+                              int configurationNotificationCount = 0;
+                              SarifHelpers.ValidateRun(

Member Author

michaelcfanning Jan 12, 2023

ValidateRun

Note that this validation will halt the test early if it fails. That's ok. If this fails, then the follow-on validation is unlikely to succeed, because we already know that we don't have the expected results + notifications data generated.

michaelcfanning commented

View reviewed changes

src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs

+                                      if (executionNotifications?.Where(t => t.Message.Text.Contains("elapsed")).Count() != expectedNotificationsCount)
+                                      {
+                                          sb.AppendLine($"\t{trace} : did not observe term 'elapsed' in rule timing notifications.");

Member Author

michaelcfanning Jan 12, 2023

\t

Notice how this code indents individual reporting lines to maximize readability in the VS test window.

michaelcfanning commented

View reviewed changes

src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs

+                                  }
+                              }
+                          }
+                          sb.Length.Should().Be(0, $"test cases failed : {Environment.NewLine}{sb}");

Member Author

michaelcfanning Jan 12, 2023

$"test cases failed : {Environment.NewLine}{sb}"

notice this construct which maximizes readability in the VS test window. this reporting subtlety should be captured in some test helpers sometime soon.

github-advanced-security bot found potential problems

View reviewed changes

src/Sarif/ExtensionMethods.cs

+                          foreach (Result result in results)
+                          {
+                              sb.AppendLine(result.ToString());

Check warning

Code scanning / CodeQL

Use of default ToString()

Default 'ToString()': [Result](1) inherits 'ToString()' from 'Object', and so is not suitable for printing. Default 'ToString()': [Result](2) inherits 'ToString()' from 'Object', and so is not suitable for printing.

src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs

		@@ -2035,6 +2145,8 @@

		public int ExpectedReturnCode;

		public int RulesCount;

Check notice

Code scanning / CodeQL

Missed 'readonly' opportunity

Field 'RulesCount' can be 'readonly'.

src/Sarif/ExtensionMethods.cs

+                          foreach (Result result in results)
+                          {
+                              sb.AppendLine(result.ToString());

Check notice

Code scanning / CodeQL

Redundant ToString() call

Redundant call to 'ToString' on a String object.

Member Author

michaelcfanning commented Jan 13, 2023 •

edited

Loading

@shaopeng, can you look at this for me? :)


          Update release notes.

a18609b

michaelcfanning merged commit 7805847 into main

michaelcfanning deleted the trace-notifications branch

January 13, 2023 17:40

Collaborator

shaopeng-gh commented Jan 13, 2023

@michaelcfanning
ah I see wrong shaopeng was added here should be shaopeng-gh so I missed this pr.
I will read the pattern and work on the memory consumption information in a similar change.

In reply to: 1381184325

Member Author

michaelcfanning commented Jan 13, 2023

Very sorry! Please review this change and let me know if you have feedback. :) we've got other worked blocked by this, so I merged already. I will remember to use the proper account reference : @shaopeng-gh . :)

This was referenced Feb 21, 2023

Releasing v3.2.0 #2622

Closed

Releasing 4.0.0 #2623

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment