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

feat: Put back telemetry #989

Merged
merged 2 commits into from
Sep 5, 2023
Merged

Conversation

JulesFaucherre
Copy link
Contributor

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have checked for similar issues and haven't found anything relevant.
  • This is not a security issue (which should be reported here: https://circleci.com/security/)
  • I have read Contribution Guidelines.

Changes

This PR aims at putting back the changes about telemetry that have been reverted in this PR
You can look at the original telemetry PR to see the discussions around the feature

Rationale

The PR also make sure the telemetry is never enabled in CI and script environment. In terms of code it means the telemetry will not be enabled when os.Getenv("CI") == true (like update check) and when stdout is not a TTY.

@JulesFaucherre JulesFaucherre changed the title feat: Added telemetry feat: Put back telemetry Aug 9, 2023
@JulesFaucherre JulesFaucherre force-pushed the feat/DEVEX-898/add-telemetry branch 5 times, most recently from 387629a to b0541fa Compare August 9, 2023 14:24
@JulesFaucherre JulesFaucherre marked this pull request as ready for review August 9, 2023 14:24
@JulesFaucherre JulesFaucherre requested review from a team as code owners August 9, 2023 14:24
This commit is a squash of many commit, if you want to retrieve the
whole history, you can checkout 3c80dfe
Telemetry is disabled when:
 - os.Getenv("CI") == "true" beacuse it means we are in a CI environment
   and we don't want to ask for telemetry approval
 - stdout is not a TTY because we can not ask for telemetry approval.
   Plus it also implies we are in a script context and we may not want
   to be prompt anything
@JulesFaucherre JulesFaucherre force-pushed the feat/DEVEX-898/add-telemetry branch from b0541fa to 0b88af7 Compare August 10, 2023 08:14
@jrahme-cci
Copy link
Contributor

This is a pretty massive PR that makes it difficult to give an honest review. Is it possible to break this up into several smaller PRs to help with that?

@JulesFaucherre
Copy link
Contributor Author

JulesFaucherre commented Sep 4, 2023

Hey @jrahme-cci , yes sorry for the massive PR, as said in the description it is a reintroduction of another PR - which is also massive... It would require a non-negligeable amount of work to split it now
If you want to look at the part that impact the runner commands, you can look at the changes in cmd/runner.
Just so you know we're completely aware of the size problem of this PR and we're gonna work to not make this happen again

@JulesFaucherre JulesFaucherre merged commit 4be7e3a into develop Sep 5, 2023
@JulesFaucherre JulesFaucherre deleted the feat/DEVEX-898/add-telemetry branch September 5, 2023 13:22
loderunner added a commit that referenced this pull request Sep 5, 2023
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.

3 participants