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.

Sign up for GitHub

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

Jump to bottom

feat: add telemetry support #136

Merged
merged 21 commits into from
May 11, 2024
Merged

feat: add telemetry support #136

merged 21 commits into from
May 11, 2024

Conversation

peterwht
Copy link
Contributor

@peterwht peterwht commented May 2, 2024 •
edited
Loading

This PR introduces ANONYMOUS telemetry to get metrics on the following things:

  • Is Pop CLI being used. This reporting only increases a counter. No personal information reported.
  • What commands are used. This, once again, increases a counter with no user inputted information reported. For example, if pop new parachain my_personal_filename is inputted, only pop new parachain will be reported.

A new arg has been added to new parachain: release_tag. This was added due to a refactor that enables calling generate_parachain_from_template once, instead of twice in separate flows. The latest release version is used if release_tag is not set. This argument will likely be most useful in a CI.

  • Telemetry handler
  • Report if CLI is used and what commands are used
  • Report generic errors (no user inputted info included)
  • Manage opt-in via config
  • Provide a way to disable telemetry will be in a new PR. Can be disabled by compiling with the no-telemetry feature flag.
  • [ ] Provide explicit explanation of how the telemetry works, what it's for, and what it tracks will be in a new PR docs(telemetry): readme explaining what and why we collect #157
  • Unit tests

@peterwht peterwht force-pushed the peter/feat-metrics branch from 9b6aa87 to 2978904 Compare May 2, 2024 20:09
@peterwht peterwht changed the title Peter/feat metrics feat: add telemetry support May 2, 2024
evilrobot-01
evilrobot-01 reviewed May 3, 2024
View reviewed changes
Copy link
Contributor

@evilrobot-01 evilrobot-01 left a comment

Choose a reason for hiding this comment

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

Know it is still a WIP, but left some comments.

Cargo.toml Show resolved Hide resolved
crates/pop-cli/Cargo.toml Show resolved Hide resolved
crates/pop-cli/src/commands/build/contract.rs Outdated Show resolved Hide resolved
crates/pop-cli/src/commands/test/contract.rs Outdated Show resolved Hide resolved
crates/pop-cli/src/main.rs Outdated Show resolved Hide resolved
crates/pop-telemetry/src/lib.rs Show resolved Hide resolved
crates/pop-telemetry/src/lib.rs Outdated Show resolved Hide resolved
crates/pop-telemetry/src/lib.rs Outdated Show resolved Hide resolved
crates/pop-telemetry/src/lib.rs Outdated Show resolved Hide resolved
crates/pop-telemetry/src/lib.rs Outdated Show resolved Hide resolved
@peterwht peterwht force-pushed the peter/feat-metrics branch from db11c0b to c7c87c2 Compare May 6, 2024 22:40
@peterwht peterwht marked this pull request as ready for review May 6, 2024 22:46
@peterwht
Copy link
Contributor Author

peterwht commented May 6, 2024

Do not merge until we have proper endpoint setup. This should be done by hardcoding the endpoint in the code instead of using the env! variable.

evilrobot-01
evilrobot-01 requested changes May 7, 2024
View reviewed changes
Copy link
Contributor

@evilrobot-01 evilrobot-01 left a comment

Choose a reason for hiding this comment

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

I think there are some improvements which can still be made. The biggest issue is the lack of a feature flag, which I think is a requirement to opt-out when running tests during CI. We dont want to have GH runs reporting telemetry data.

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
crates/pop-telemetry/Cargo.toml Outdated Show resolved Hide resolved
crates/pop-telemetry/Cargo.toml Outdated Show resolved Hide resolved
crates/pop-cli/src/commands/build/contract.rs Outdated Show resolved Hide resolved
crates/pop-cli/src/main.rs Outdated Show resolved Hide resolved
crates/pop-telemetry/src/lib.rs Outdated Show resolved Hide resolved
crates/pop-telemetry/src/lib.rs Outdated Show resolved Hide resolved
crates/pop-telemetry/src/lib.rs Outdated Show resolved Hide resolved
crates/pop-telemetry/src/lib.rs Outdated Show resolved Hide resolved
@peterwht peterwht requested a review from evilrobot-01 May 7, 2024 22:55
@AlexD10S AlexD10S requested a review from brunopgalvao May 8, 2024 08:01
AlexD10S
AlexD10S reviewed May 8, 2024
View reviewed changes
Copy link
Collaborator

@AlexD10S AlexD10S left a comment

Choose a reason for hiding this comment

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

Looks great! I just left a couple of comments.
Can't wait to see some metrics!

crates/pop-cli/src/commands/new/parachain.rs Show resolved Hide resolved
crates/pop-telemetry/src/lib.rs Outdated Show resolved Hide resolved
@evilrobot-01
refactor: improve feature (#145)
c802275 
* test: ensure errors propagated (#143)

* test: relocate integration tests (#144)

* test: relocate integration tests

* ci: reduce run time

* refactor: remove unused import

* refactor: improve feature isolation

* refactor: remove unnecessary path prefixes

* refactor: invert feature

Using `--features=no_telemetry` still resulted in inclusion of pop-telemetry in the dependency tree. Using `telemetry` also makes conditional compilation much simpler.
brunopgalvao
brunopgalvao reviewed May 10, 2024
View reviewed changes
Copy link
Contributor

@brunopgalvao brunopgalvao left a comment

Choose a reason for hiding this comment

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

Looks like the Github Action check has not completed. The PR LGTM. Let's get the checks to pass.

evilrobot-01
evilrobot-01 requested changes May 10, 2024
View reviewed changes
Copy link
Contributor

@evilrobot-01 evilrobot-01 left a comment

Choose a reason for hiding this comment

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

Seems to be regressions, presumably due to resolvign merged conflicts.

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
crates/pop-cli/Cargo.toml Outdated Show resolved Hide resolved
@peterwht peterwht requested a review from evilrobot-01 May 10, 2024 21:16
@peterwht peterwht mentioned this pull request May 10, 2024
Merged
evilrobot-01
evilrobot-01 approved these changes May 11, 2024
View reviewed changes
Copy link
Contributor

@evilrobot-01 evilrobot-01 left a comment

Choose a reason for hiding this comment

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

LGTM!

@AlexD10S AlexD10S merged commit 0999d65 into main May 11, 2024
10 checks passed
@AlexD10S AlexD10S deleted the peter/feat-metrics branch May 11, 2024 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brunopgalvao brunopgalvao brunopgalvao left review comments

@evilrobot-01 evilrobot-01 evilrobot-01 approved these changes

@AlexD10S AlexD10S AlexD10S approved these changes

@weezy20 weezy20 Awaiting requested review from weezy20

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@peterwht @brunopgalvao @AlexD10S @evilrobot-01