-
Notifications
You must be signed in to change notification settings - Fork 218
Telemetry for function-testing-helpers usage tracking #6509
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
Conversation
Coverage report
Show files with reduced coverage 🔻
Test suite run success3345 tests passing in 1371 suites. Report generated by 🧪jest coverage report action from 8f7a779 |
d79a2b2 to
e6f8368
Compare
| 'invoked-by': Flags.string({ | ||
| hidden: true, | ||
| description: 'Internal flag to track which client invoked this command.', | ||
| env: 'SHOPIFY_FLAG_INVOKED_BY', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually really great because in the helper, we can set this environment variable and old CLI versions will just ignore it!
|
Does this need to be a full flag? You can just use the env var no? |
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
b507047 to
3a5f9e6
Compare
| description: 'Name of the WebAssembly export to invoke.', | ||
| env: 'SHOPIFY_FLAG_EXPORT', | ||
| }), | ||
| 'invoked-by': Flags.string({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this flag should have hidden: true as it is not intended to be used externally
8955c1f to
8ce1f05
Compare
|
Actually @isaacroldan I realized that for developers with old version of CLI using shopify-function-test-helpers the |
e17bd88 to
6a079ee
Compare
| env_shopify_variables: JSON.stringify(Object.fromEntries(getShopifyEnvironmentVariables())), | ||
| } | ||
| } | ||
|
|
||
| function getShopifyEnvironmentVariables() { | ||
| const shopifyEnvVars = new Map<string, string>() | ||
| for (const [key, value] of Object.entries(process.env)) { | ||
| if (key.startsWith('SHOPIFY_') && value !== undefined) { | ||
| shopifyEnvVars.set(key, value) | ||
| } | ||
| } | ||
| return shopifyEnvVars | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just make getShopifyEnvironmentVariables return a JSON, this function can be simplified to a single line:
Object.fromEntries(Object.entries(process.env).filter(([key]) => key.startsWith('SHOPIFY_')))6a079ee to
afdce2f
Compare
lopert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the env var approach! Thanks for digging for it! 🙏
541b48a to
8f7a779
Compare
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/private/node/analytics.d.ts@@ -27,5 +27,6 @@ interface EnvironmentData {
export declare function getEnvironmentData(config: Interfaces.Config): Promise<EnvironmentData>;
export declare function getSensitiveEnvironmentData(config: Interfaces.Config): Promise<{
env_plugin_installed_all: string;
+ env_shopify_variables: string;
}>;
export {};
\ No newline at end of file
packages/cli-kit/dist/public/node/metadata.d.ts@@ -55,6 +55,7 @@ declare const coreData: RuntimeMetadataManager<CmdFieldsFromMonorail, {
cmd_all_environment_flags?: string | null;
cmd_dev_tunnel_custom?: string | null;
env_plugin_installed_all?: string | null;
+ env_shopify_variables?: string | null;
}, "store_", never>>;
export declare const getAllPublicMetadata: () => Partial<CmdFieldsFromMonorail>, getAllSensitiveMetadata: () => Partial<{
commandStartOptions: {
@@ -74,6 +75,7 @@ export declare const getAllPublicMetadata: () => Partial<CmdFieldsFromMonorail>,
cmd_all_environment_flags?: string | null;
cmd_dev_tunnel_custom?: string | null;
env_plugin_installed_all?: string | null;
+ env_shopify_variables?: string | null;
}, "store_", never>>, addPublicMetadata: (getData: ProvideMetadata<CmdFieldsFromMonorail>, onError?: MetadataErrorHandling) => Promise<void>, addSensitiveMetadata: (getData: ProvideMetadata<{
commandStartOptions: {
startTime: number;
@@ -92,6 +94,7 @@ export declare const getAllPublicMetadata: () => Partial<CmdFieldsFromMonorail>,
cmd_all_environment_flags?: string | null;
cmd_dev_tunnel_custom?: string | null;
env_plugin_installed_all?: string | null;
+ env_shopify_variables?: string | null;
}, "store_", never>>, onError?: MetadataErrorHandling) => Promise<void>, runWithTimer: (field: NumericKeyOf<CmdFieldsFromMonorail>) => <T>(fn: () => Promise<T>) => Promise<T>;
export type Public = PublicSchema<typeof coreData>;
export type Sensitive = SensitiveSchema<typeof coreData>;
packages/cli-kit/dist/public/node/monorail.d.ts@@ -2,7 +2,7 @@ import { JsonMap } from '../../private/common/json.js';
import { DeepRequired } from '../common/ts/deep-required.js';
export { DeepRequired };
type Optional<T> = T | null;
-export declare const MONORAIL_COMMAND_TOPIC = "app_cli3_command/1.19";
+export declare const MONORAIL_COMMAND_TOPIC = "app_cli3_command/1.20";
export interface Schemas {
[MONORAIL_COMMAND_TOPIC]: {
sensitive: {
@@ -14,6 +14,7 @@ export interface Schemas {
cmd_all_environment_flags?: Optional<string>;
cmd_dev_tunnel_custom?: Optional<string>;
env_plugin_installed_all?: Optional<string>;
+ env_shopify_variables?: Optional<string>;
};
public: {
business_platform_id?: Optional<number>;
|
WHY are these changes introduced?
Fixes: https://github.com/shop/issues-shopifyvm/issues/630
We want telemetry on usage and adoption of shopify-function-test-helpers. We will do so by setting an
SHOPIFY_INVOKED_BYenv variable to thefunction runorfunction buildcommand when invoking fromshopify-function-test-helpers. This value will then be passing to monorail through aenv_shopify_variablesfield of the monorail payload.WHAT is this pull request doing?
invoked-byflag to function run command.How to test your changes?
I added a unit test, I dont know if we can tophat this change locally but once its out, we should be able to query this column in the sdp tables in BigQuery.
Post-release steps
Measuring impact
How do we know this change was effective? Please choose one:
Checklist