feat(telemetry): add wasm-tools wasm binary annotations #1016
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Integralist
force-pushed
the
integralist/wasm-tools
branch
from
06cd1f9 to
18228fd
Compare
joeshaw
reviewed
Sep 22, 2023
Comment on lines
+412
to
+435
| platform := runtime.GOOS | ||
| if platform == "darwin" { | ||
| platform = "macos" | ||
| } | ||
|
|
||
| arch := runtime.GOARCH | ||
| if arch == "arm64" { | ||
| arch = "aarch64" | ||
| } |
There was a problem hiding this comment.
Unfortunately this sort of thing isn't really consistent from project to project, so there's a good chance you'll have to make additional special cases in the future for other tools. But since this only applies to wasm-tools for now, it's fine.
There was a problem hiding this comment.
Yup, agreed. This was designed to work specifically for wasm-tools and I think somewhere in the code comments I even say something like "once we add more tools this pattern has to be abstracted".
jaskiratr
approved these changes
Sep 22, 2023
Integralist
force-pushed
the
integralist/wasm-tools
branch
21 times, most recently
from
00d2b45 to
33f6947
Compare
This is because we now always annotate the Wasm binary. If the user enables metadata then we annotate lots of data, otherwise we'll default to annotating the binary with the CLI version and the project language.
Integralist
force-pushed
the
integralist/wasm-tools
branch
from
5d81128 to
1099ab1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The intention is for this PR to be merged before #1013. Once this PR is merged (read: merged but NOT released), then the latter PR can be hooked into the metadata collection flows.
Context
Ultimately if we did publish both this PR and the latter as part of a new CLI release, they would still not collect any data yet as the annotations (which are part of this PR) are locked down to a feature flag to prevent users from accidentally sending metadata without the proper/appropriate consent.
Also worth noting there is nothing on the Fastly platform side to actually consume this metadata, so nothing would be 'analysed' until appropriate communications had been sent out and users given adequate time to adjust to the upcoming CLI release.
Although the metadata collection is behind a feature flag, this PR does still support disabling all metadata using an environment variable
FASTLY_WASM_METADATA_DISABLE=true. This is just to be prepared for an eventual CLI release where the metadata collection becomes opt-out and so this will be required for adding to CI environments like the Fastly Compute Actions repo that customers use for installing the CLI and compiling/deploying Compute projects.There should be a third PR at some point to remove the feature flag, so that the annotations are dependent solely on the user configuring their consent/non-consent via
fastly compute metadata(which is implemented in the latter PR).Screenshots
The following screenshot shows the default metadata on a compiled Wasm binary (i.e. metadata HAS NOT been enabled)...
The following screenshot shows the metadata attached when passing the
--metadata-enableflag (i.e. metadata collection HAS BEEN opted into)...The following screenshot demonstrates that even if I set
--metadata-enableno metadata is collected because I've also setFASTLY_WASM_METADATA_DISABLE=true, so although it's an unlikely situation, we always err on the side of caution and will give precedence to disabling over the flag (typically flags are given highest priority, but I've opted against that here)...