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

Send tools telemetry to init status report #1497

Merged
merged 17 commits into from
Jan 25, 2023

Conversation

angelapwen
Copy link
Contributor

@angelapwen angelapwen commented Jan 21, 2023

This PR sends the following fields to the init status report on success and failure:

  • tools_source
  • if tools_source was DOWNLOAD, tools_download_duration_ms and tools_feature_flags_valid

Note that previously we only sent additional fields on init success, but because we would like to support the tools fields on failure as well, the PR changes the status report behavior so that the init report is sent on failure too, with as many fields populated as possible.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@angelapwen angelapwen force-pushed the tools-telemetry-report branch from a9545a6 to f24d78c Compare January 21, 2023 00:37
@angelapwen angelapwen changed the title Send tools telemetry to init success status report Send tools telemetry to init status report Jan 21, 2023
@angelapwen angelapwen marked this pull request as ready for review January 21, 2023 01:06
@angelapwen angelapwen requested a review from a team as a code owner January 21, 2023 01:06
@angelapwen angelapwen force-pushed the tools-telemetry-report branch from a217825 to 4fa58ed Compare January 21, 2023 01:10
@angelapwen
Copy link
Contributor Author

Looks like a bit of an inscrutable error, init action failed: TypeError: (0 , actions_util_1.getActionVersion) is not a function when it is correctly defined in actions-util and I haven't changed anything about it.

Also, oddly enough the error fails in the analyze step even though it originates from init

@henrymercer
Copy link
Contributor

I think this could be as boring as just needing to merge in main and recompile the Action — some of the compiled files seem to be missing at the moment.

src/init.ts Outdated Show resolved Hide resolved
src/init-action.ts Fixed Show fixed Hide fixed
src/init-action.ts Fixed Show fixed Hide fixed
src/init-action.ts Fixed Show fixed Hide fixed
src/init-action.ts Fixed Show fixed Hide fixed
@angelapwen
Copy link
Contributor Author

I have an issue with node in VSCode so the automated linting isn't working very well.. trying to resolve now.

@angelapwen angelapwen force-pushed the tools-telemetry-report branch from 8ca90f9 to 919c531 Compare January 23, 2023 18:30
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

I like how you've used this additional telemetry information to improve the tests for setting up the CodeQL tools! Here's a couple of initial comments:

src/codeql.test.ts Outdated Show resolved Hide resolved
src/init-action.ts Outdated Show resolved Hide resolved
src/init-action.ts Outdated Show resolved Hide resolved
src/init-action.ts Outdated Show resolved Hide resolved
@angelapwen
Copy link
Contributor Author

I think I've addressed all comments and PR is ready for review again!

src/feature-flags.ts Outdated Show resolved Hide resolved
src/init-action.ts Outdated Show resolved Hide resolved
src/init-action.ts Outdated Show resolved Hide resolved
src/init-action.ts Outdated Show resolved Hide resolved
src/init-action.ts Outdated Show resolved Hide resolved
src/init-action.ts Outdated Show resolved Hide resolved
henrymercer
henrymercer previously approved these changes Jan 25, 2023
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

Great stuff! A couple of minor suggestions to improve the types, and there are a few conflicts to resolve before we can merge this.

src/feature-flags.ts Show resolved Hide resolved
src/feature-flags.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

🎉

@angelapwen angelapwen merged commit 24ca6b0 into github:main Jan 25, 2023
@angelapwen angelapwen deleted the tools-telemetry-report branch January 25, 2023 19:09
@github-actions github-actions bot mentioned this pull request Jan 26, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants