Skip to content

Conversation

@hvitved
Copy link
Contributor

@hvitved hvitved commented Feb 6, 2025

For now, this will yield DCA numbers on missing call targets, but should also eventually yield telemetry data.

@github-actions github-actions bot added C# Java Rust Pull requests that update Rust code labels Feb 6, 2025
@hvitved hvitved marked this pull request as ready for review February 6, 2025 13:34
Copilot AI review requested due to automatic review settings February 6, 2025 13:34
@hvitved hvitved requested review from a team as code owners February 6, 2025 13:34
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

@hvitved hvitved added the no-change-note-required This PR does not need a change note label Feb 6, 2025
michaelnebel
michaelnebel previously approved these changes Feb 6, 2025
Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

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

C# LGTM

@hvitved hvitved requested a review from geoffw0 February 7, 2025 07:59
@@ -0,0 +1,46 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have a rust/ql/src/queries/diagnostics and rust/ql/src/queries/summary directory and this introduces rust/ql/src/queries/telemetry as well. Are the new queries meaningfully different enough to justify a third location or should we condense back down to (any) two of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I simply followed the same structure that we have for Java and C#.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I'll look into whether I can combine any of these directories after this PR is merged. Don't really want a proliferation of folders unless they're meaningfully different.

MacroCallTargetStatsReport::numberOfOk(key, value) or
MacroCallTargetStatsReport::numberOfNotOk(key, value) or
MacroCallTargetStatsReport::percentageOfOk(key, value)
) and
Copy link
Contributor

Choose a reason for hiding this comment

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

There's quite a bit of overlap with rust/summary/summary-statistics (files and lines of code extracted), the focus is a little different here but something to be aware of.

/* -Infinity */
value != -1.0 / 0.0 and
/* NaN */
value != 0.0 / 0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

This is hurting my maths brain, but it does seem to be the way float works. :(

@geoffw0
Copy link
Contributor

geoffw0 commented Feb 7, 2025

DCA LGTM, I can see the new "Missing call targets, per source"

Co-authored-by: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
@hvitved hvitved merged commit 614b3ce into github:main Feb 7, 2025
45 checks passed
@hvitved hvitved deleted the rust/telemetry branch February 7, 2025 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C# Java no-change-note-required This PR does not need a change note Rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants