Skip to content
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.

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

Add a filter for faults to avoid bucketing #10921

Merged

Conversation

ryzngard
Copy link
Contributor

Helps remove AssumeNotNull from the fault parameters and blame something in our code stack instead

@ryzngard ryzngard requested a review from a team as a code owner September 24, 2024 23:37
@@ -16,6 +17,15 @@ namespace Microsoft.VisualStudio.Razor.Telemetry;

internal abstract class TelemetryReporter : ITelemetryReporter
{
private const string CodeAnalysisNamespace = nameof(Microsoft) + "." + nameof(CodeAnalysis);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're going to have a half dozen of these where we want to report the caller; is there a unittest to write to make sure we are detecting/rewriting info as we expect?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I didn't see a way to check the properties being set here. @pieandcakes do you know?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll ask a different way. Are there reasonable unittests for GetModifiedFaultParameters to prove it extracts the stack top OR the correct depth within it, and when we add 'more' examples of frames to skip, we keep the same behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That I can add, but if we can find a way to assert the fault behavior I would prefer that. If not testing GetModifiedFaultParameters is still a good test.

Copy link
Contributor

@pieandcakes pieandcakes Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't process the bucket parameters until PostEvent is called and then it processes them and adds them to the properties. I don't think you'd have a way to validate that the values are correct/not correct .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Should the Module be something like Microsoft.VisualStudio.LanguageServices.Razor.Test.dll or should it be the containing type full name?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryzngard ryzngard force-pushed the filter_exceptions_for_faults branch from 61cb332 to 1676a54 Compare September 25, 2024 19:07
@ryzngard ryzngard requested a review from a team as a code owner September 25, 2024 19:07
@ryzngard ryzngard changed the base branch from main to release/dev17.12 September 25, 2024 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants