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(compute): add metadata subcommand #1013

Merged
merged 19 commits into from
Oct 31, 2023
Merged

feat(compute): add metadata subcommand #1013

merged 19 commits into from
Oct 31, 2023

Conversation

Integralist
Copy link
Collaborator

@Integralist Integralist commented Sep 18, 2023 •
edited
Loading

This PR introduces a new fastly compute metadata command.
It should only be merged after #1016 is merged.

NOTE: No metadata is currently collected.
This PR simply introduces the options first, then in a later PR we will implement the metadata calls.

Below are some screenshots highlighting how the new compute metadata command works...

NOTE: These screenshots were taken back when the command was a top-level telemetry command. We've since changed it to be nested as metadata under the compute command because we're not instrumenting the CLI. We're simply annotating the Wasm binary and so we don't want to mislead users into thinking this is something bigger than it is.

Screenshot 2023-09-18 at 14 47 51 Screenshot 2023-09-18 at 14 48 05

Below is a screenshot highlighting the new compute metadata messaging for users who install the CLI for the first time or who upgrade to the CLI version that includes metadata collection...

Monosnap cts:testing-fastly-cli 2023-09-21 15-48-50

@Integralist Integralist added the enhancement New feature or request label Sep 18, 2023
@Integralist Integralist force-pushed the integralist/telemetry branch 2 times, most recently from 3420dad to a516765 Compare September 18, 2023 15:27
@Integralist Integralist marked this pull request as draft September 19, 2023 08:13
@Integralist
Copy link
Collaborator Author

I've converted this PR to draft until we've had time to agree on what the 'default' behaviour is going to be and also the sequencing of events to the rollout of this feature.

@Integralist Integralist mentioned this pull request Sep 20, 2023
Merged
@Integralist Integralist force-pushed the integralist/telemetry branch 2 times, most recently from 2cdf8ae to db01ef9 Compare September 26, 2023 10:14
@Integralist Integralist changed the title feat(telemetry): add telemetry command feat(compute/metadata): add compute metadata command Sep 26, 2023
@Integralist Integralist changed the title feat(compute/metadata): add compute metadata command feat(compute): add compute metadata command Sep 26, 2023
@Integralist Integralist changed the title feat(compute): add compute metadata command feat(compute): add metadata subcommand Sep 26, 2023
@Integralist Integralist marked this pull request as ready for review September 26, 2023 11:01
joeshaw
joeshaw reviewed Sep 29, 2023
View reviewed changes
Copy link
Member

@joeshaw joeshaw left a comment

Choose a reason for hiding this comment

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

LGTM except for the FIXME for the URL. One suggestion below if you like it.

pkg/commands/compute/metadata.go Show resolved Hide resolved
@Integralist Integralist force-pushed the integralist/telemetry branch from 0deae51 to de810cb Compare October 2, 2023 10:49
@Integralist
Copy link
Collaborator Author

@joeshaw PR has been amended as per your feedback.

The new output will look something like...

Screenshot 2023-10-02 at 11 52 31

@Integralist Integralist requested a review from joeshaw October 2, 2023 10:53
@Integralist Integralist force-pushed the integralist/telemetry branch from e8e6f65 to 502108b Compare October 2, 2023 17:03
@joeshaw
Copy link
Member

joeshaw commented Oct 2, 2023

Thanks. LGTM other than the FIXME.

@Integralist Integralist force-pushed the integralist/telemetry branch 4 times, most recently from 3f8a4de to b2e7a99 Compare October 11, 2023 12:17
@Integralist Integralist force-pushed the integralist/telemetry branch 4 times, most recently from 62d3487 to 2d71aa2 Compare October 18, 2023 11:56
@Integralist Integralist force-pushed the integralist/telemetry branch from 2d71aa2 to 59d1866 Compare October 25, 2023 08:25
@Integralist Integralist force-pushed the integralist/telemetry branch from 1dded33 to 6f28879 Compare October 25, 2023 13:19
@Integralist Integralist force-pushed the integralist/telemetry branch from 6f28879 to 9f5b4dd Compare October 31, 2023 17:28
Integralist
Integralist commented Oct 31, 2023
View reviewed changes
pkg/app/run.go Outdated
Comment on lines 118 to 119
// FIXME: We need the actual URL to point users to.
text.Important(g.Output, "The Fastly CLI is configured to collect data related to Wasm builds (e.g. compilation times, resource usage, and other non-identifying data). To learn more about what data is being collected, why, and how to disable it: https://www.fastly.com/")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jaskiratr we'll need an actual URL here (or should this be https://bit.ly/wasm-metadata which points to our community forum thread on the topic)?

Copy link

Choose a reason for hiding this comment

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

We'll update the CLI reference page with the information about what data is being collected, why, and how to disable it

@Integralist Integralist force-pushed the integralist/telemetry branch from 27b1956 to e35ebba Compare October 31, 2023 19:40
@Integralist Integralist merged commit 2aeed0f into main Oct 31, 2023
6 checks passed
@Integralist Integralist deleted the integralist/telemetry branch October 31, 2023 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joeshaw joeshaw joeshaw approved these changes

@jaskiratr jaskiratr Awaiting requested review from jaskiratr

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Integralist @joeshaw @jaskiratr