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

Rust: Implement database quality telemetry query #18697

Merged
merged 2 commits into from
Feb 7, 2025

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 Copilot bot 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

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"

@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