Skip to content

Conversation

@akhera99
Copy link
Member

Adds some telemetry to see if documentation comments impact the number of users clicking "tell me more" link.

@akhera99 akhera99 requested a review from genlu June 20, 2024 22:24
@ghost ghost added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 20, 2024
@akhera99 akhera99 marked this pull request as ready for review June 21, 2024 17:32
@akhera99 akhera99 requested a review from a team as a code owner June 21, 2024 17:32
throw new InvalidOperationException("QuickInfoSession is null");
}

OnTheFlyDocsLogger.LogShowedOnTheFlyDocsLink();
Copy link
Member

@genlu genlu Jun 21, 2024

Choose a reason for hiding this comment

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

Could we log whether a symbol has doc-comment here? That would tell us how often a symbol user hover over has no doc, right?

/// <param name="language">the language of the symbol</param>
internal sealed class OnTheFlyDocsElement(string symbolSignature, ImmutableArray<string> declarationCode, string language)
/// <param name="hasComments">whether the symbol has comments</param>
internal sealed class OnTheFlyDocsElement(string symbolSignature, ImmutableArray<string> declarationCode, string language, bool hasComments)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe also differentiate source symbols and metadata symbols?

Logger.Log(FunctionId.CodeAnalysisService_CalculateDiagnosticsAsync, KeyValueLogMessage.Create(m =>
{
m["SymbolHeaderText"] = _onTheFlyDocsElement.SymbolSignature;
}, LogLevel.Information));
Copy link
Member

Choose a reason for hiding this comment

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

what's this log message for?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I meant to change that

return new OnTheFlyDocsElement(symbol.ToDisplayString(), symbolStrings, symbol.Language);
return new OnTheFlyDocsElement(symbol.ToDisplayString(), symbolStrings, symbol.Language, hasDocumentationComment);

bool HasDocumentationComment(ImmutableArray<SyntaxReference> references)
Copy link
Member

Choose a reason for hiding this comment

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

when creating a tooltip in QuickInfoUtilities.CreateQuickInfoItemAsync, it looks like we know whether if contains doc-comment, is it possible log there instead so we don't have to check the syntax?

return null;
}

if (symbol.MetadataToken != 0)
Copy link
Member

Choose a reason for hiding this comment

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

so this does what we want? Great!

@akhera99 akhera99 enabled auto-merge (squash) June 21, 2024 23:09
@akhera99 akhera99 merged commit 847d588 into dotnet:main Jun 22, 2024
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jun 22, 2024
@jjonescz jjonescz modified the milestones: Next, 17.11 P3 Jun 24, 2024
333fred added a commit to 333fred/roslyn that referenced this pull request Jun 24, 2024
* upstream/main:
  [main] Update dependencies from dotnet/arcade (dotnet#74099)
  Remove warning for `yield return` in `lock` (dotnet#74024)
  [release/dev17.10] Update dependencies from dotnet/arcade (dotnet#74113)
  On the fly docs - add telemetry regarding documentation comments (dotnet#74088)
  Query for the COM service provider instead of direct cast
  Add type hints for collection expressions (dotnet#74051)
  Align implementation with latest LDM decisions around invocations in presence of dynamic arguments. (dotnet#74097)
  Avoid allocations in AbstractSyntaxIndex<>.GetIndexAsync
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants