C++: Initial telemetry queries #17892

calumgrant · 2024-11-01T16:44:22Z

This adds new internal telemetry queries, so should not need a changenote.

It loosely follows the examples in https://github.com/github/codeql/tree/main/java/ql/src/Telemetry and https://github.com/github/codeql/tree/main/csharp/ql/src/Telemetry.

As a new idea, Metrics.qll uses QL classes a bit more heavily, instead of binary predicates, but obviously they boil down to the same thing.

We don't (yet) emit a diagnostic warning about low quality databases.

Note that qltest can't test for missing includes, since these lead to a catastrophic error by default.

Fixes https://github.com/github/codeql-c-team/issues/2471

Pull Request checklist

All query authors

~~[ ] A change note is added if necessary. See the documentation in this repository.~~
~~[ ] All new queries have appropriate .qhelp. See the documentation in this repository.~~
QL tests are added if necessary. See Testing custom queries in the GitHub documentation.
New and changed queries have correct query metadata. See the documentation in this repository.

Internal query authors only

~~[ ] Autofixes generated based on these changes are valid, only needed if this PR makes significant changes to .ql, .qll, or .qhelp files. See the documentation (internal access required).~~
Changes are validated at scale (internal access required).
~~[ ] Adding a new query? Consider also adding the query to autofix.~~

jketema

Is there are specific reason we're not re-using some of the functionality already present in cpp/ql/src/Diagnostics?

jketema · 2024-11-04T09:54:04Z

cpp/ql/test/library-tests/extraction_errors/CompilerErrors.expected

+| identifier 'nsf2' is undefined | 1.0 |
+| identifier 'so_is_this' is undefined | 1.0 |
+| identifier 'uint32_t' is undefined | 1.0 |
+| too few arguments in function call | 1.0 |


Why are the values here floats? It seems like these are just discrete counts.

It's because all metrics are floats in general. But it might be worth specialising to int-metrics and float-metrics.

That seems a very bad design.

Is 841c678 any better?

calumgrant · 2024-11-04T10:18:52Z

Is there are specific reason we're not re-using some of the functionality already present in cpp/ql/src/Diagnostics?

The code in cpp/ql/src/Diagnostics isn't really that helpful, mainly because it's in the wrong format and has @kind diagnostic.

jketema · 2024-11-04T10:41:30Z

Is there are specific reason we're not re-using some of the functionality already present in cpp/ql/src/Diagnostics?

The code in cpp/ql/src/Diagnostics isn't really that helpful, mainly because it's in the wrong format and has @kind diagnostic.

Not even the libraries in that directory are of use?

jketema

Some questions about the used tags.

jketema · 2024-11-04T10:43:34Z

cpp/ql/src/Telemetry/Diagnostics.qll

+ * Typically this is due to a missing include.
+ */
+class CannotOpenFile extends CompilerError {
+  CannotOpenFile() { this.hasTag("cannot_open_file") }


Shouldn't this also cover cannot_open_file_reason?

I will have a look at these tags 👍

jketema · 2024-11-04T10:43:38Z

cpp/ql/src/Telemetry/Diagnostics.qll

+ * Currently unused.
+ */
+class UndefinedIdentifier extends CompilerError {
+  UndefinedIdentifier() { this.hasTag("undefined_identifier") }


There are a number of other ..._undefined_identifier tags, shouldn't those be covered too?

I've had a search through the front end source code, and the other ec_undefined_ errors are all at warning or remark level (e.g. ec_undefined_preproc_id)

What about ec_range_based_for_undefined_identifier and ec_for_each_undefined_identifier?

Ok, I think I'll just delete UndefinedIdentifier as it's unused and not worth creating a test for.

jketema · 2024-11-04T10:53:23Z

cpp/ql/src/Telemetry/Diagnostics.qll

+ * A syntax error.
+ */
+class SyntaxError extends CompilerError {
+  SyntaxError() { this.getTag().matches("exp_%") }


Does exp_ cover all possible syntax errors?

Ah, no. Searching for syntax_error(ec_ reveals various other syntax errors that I will add.

calumgrant · 2024-11-04T11:18:38Z

Is there are specific reason we're not re-using some of the functionality already present in cpp/ql/src/Diagnostics?

The code in cpp/ql/src/Diagnostics isn't really that helpful, mainly because it's in the wrong format and has @kind diagnostic.

Not even the libraries in that directory are of use?

Well, telemetry is a bit simpler because it's a question of dumping counts and raw data, so we don't need to refine them in the same way.

jketema · 2024-11-08T10:49:12Z

cpp/ql/src/Telemetry/MissingIncludes.ql

+from CppMetrics::MissingIncludeCount e
+select e.getIncludeText(), e.getValue()


I'm a bit concerned that this might completely blow up on real-world projects to such an extent that the SARIF will be rejected when uploaded.

Makes sense. I've limited the results to the top 500 - not sure if that's a sensible number or not.

jketema · 2024-11-08T10:49:57Z

cpp/ql/src/Telemetry/CompilerErrors.ql

+from CppMetrics::ErrorCount m
+select m.toString(), m.getValue()


I'm a bit concerned that this might completely blow up on real-world projects to such an extent that the SARIF will be rejected when uploaded. Especially because the error messages can in principle be several lines long.

C++: Initial telemetry queries and tests

8deed3e

github-actions bot added the C++ label Nov 1, 2024

calumgrant force-pushed the calumgrant/telemetry branch from 9409641 to 913056a Compare November 1, 2024 17:01

C++: Minor edits

8035a7c

calumgrant force-pushed the calumgrant/telemetry branch from 913056a to 8035a7c Compare November 1, 2024 17:08

calumgrant added 2 commits November 1, 2024 17:15

C++: Minor edits

d5940b2

C++: Minor edits

8506fd0

calumgrant marked this pull request as ready for review November 4, 2024 09:28

calumgrant requested a review from a team as a code owner November 4, 2024 09:28

jketema reviewed Nov 4, 2024

View reviewed changes

calumgrant added the no-change-note-required This PR does not need a change note label Nov 4, 2024

calumgrant added 2 commits November 4, 2024 14:03

C++: Separate int and float metrics

841c678

C++: Match more tags

ac692ee

calumgrant force-pushed the calumgrant/telemetry branch from 201f282 to ac692ee Compare November 4, 2024 14:04

jketema reviewed Nov 8, 2024

View reviewed changes

C++: Limit metrics to top 500

8029eb2

calumgrant force-pushed the calumgrant/telemetry branch from 4fc90ca to 8029eb2 Compare November 8, 2024 14:38

C++: Remove unused class UndefinedIdentifier

fa55dd1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C++: Initial telemetry queries #17892

C++: Initial telemetry queries #17892

calumgrant commented Nov 1, 2024 •

edited

Loading

jketema left a comment

jketema Nov 4, 2024

calumgrant Nov 4, 2024

jketema Nov 4, 2024

calumgrant Nov 4, 2024

calumgrant commented Nov 4, 2024

jketema commented Nov 4, 2024

jketema left a comment

jketema Nov 4, 2024

calumgrant Nov 4, 2024

jketema Nov 4, 2024

calumgrant Nov 4, 2024 •

edited by jketema

Loading

jketema Nov 8, 2024

calumgrant Nov 8, 2024

jketema Nov 4, 2024

calumgrant Nov 4, 2024

calumgrant commented Nov 4, 2024

jketema Nov 8, 2024

calumgrant Nov 8, 2024

jketema Nov 8, 2024

		from CppMetrics::MissingIncludeCount e
		select e.getIncludeText(), e.getValue()

		from CppMetrics::ErrorCount m
		select m.toString(), m.getValue()

C++: Initial telemetry queries #17892

Are you sure you want to change the base?

C++: Initial telemetry queries #17892

Conversation

calumgrant commented Nov 1, 2024 • edited Loading

Pull Request checklist

All query authors

Internal query authors only

jketema left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calumgrant commented Nov 4, 2024

jketema commented Nov 4, 2024

jketema left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calumgrant Nov 4, 2024 • edited by jketema Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calumgrant commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calumgrant commented Nov 1, 2024 •

edited

Loading

calumgrant Nov 4, 2024 •

edited by jketema

Loading