Skip to content

C#: Telemetry query updates. #11083

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

Merged
merged 7 commits into from
Nov 11, 2022
Merged

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Nov 2, 2022

It turns out that the Telemetry we emit for API calls in C# needs to be changed.

  • We have removed the (assembly) file name as the same method can be located in different assemblies across version, which clutters the picture of the usage.
  • We have aligned the counting to only count calls and not the possible (runtime) dispatch targets.
  • We now only consider callables that are effectively public.

This should align the implemetation a bit better with Java.

With respect to testing of the PR

  • The unit tests behaves as expected (the coverage is not optimal, but still a good indication that everything works as expected).
  • I have tried to run some of the queries locally against .NET Runtime and .NET EfCore and the results looks reasonable (ie. the results doesn't contain any "noise", but it would be a huge task to verify that the results are correct manually).
  • The DCA results looks acceptable (ie. there are no performance regressions when running the full nightly query suite).

The only thing in this PR I am a bit uncertain about is, whether it is a good idea to tag some of the string generating predicates with the bindingset pragma.

@github-actions github-actions bot added the C# label Nov 2, 2022
@michaelnebel michaelnebel marked this pull request as ready for review November 4, 2022 07:52
@michaelnebel michaelnebel requested a review from a team as a code owner November 4, 2022 07:52
@michaelnebel michaelnebel added the no-change-note-required This PR does not need a change note label Nov 4, 2022
Copy link
Contributor

@sidshank sidshank left a comment

Choose a reason for hiding this comment

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

Based on the async chats we've had about this PR, looks plausible to me!

@michaelnebel michaelnebel merged commit ef50e57 into github:main Nov 11, 2022
@michaelnebel michaelnebel deleted the csharp/telemetry branch November 11, 2022 09:57
@michaelnebel
Copy link
Contributor Author

Based on the async chats we've had about this PR, looks plausible to me!

Great - thank you for the review! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants