@cj-radcliff @kenzieschmoll @bwilkerson @pq This PR adds the env vars discussed previously (minus DASH__TOOL because that'd TBD).

The vars are set for all processes we spawn (this includes Analysis server, Flutter, Pub, DevTools server), but not for the built-in terminal.

Let me know if there's anything else we should confirm/check before landing these.

If we're happy with them, we should also add the equivalent to the IJ plugins for all tool processes that are spawned (cc @helin24).

That looks good to me. Thanks!

Will DASH__IDE_NAME be passed to the DevTools query string and the flutter survey data as a follow up to this PR?

@kenzieschmoll

Will DASH__IDE_NAME be passed to the DevTools query string and the flutter survey data as a follow up to this PR?

For surveys - if that's what makes most sense and they both accept arbitrary strings (not just the specific values we pass today), sure.

For DevTools, I'm wondering whether we should stop using* the querystring and instead have the server read these (and whatever DASH__TOOL becomes), so it has all of the information and can record the same things as other tools like the analysis server. I think ideally, all tools (DevTools server, Flutter, Pub, Analysis server) would know about these env vars, and report to a unified analytics consistently.

* And by stop using, I mean continue to pass it, but update the server to use the new env variables when set, but fall back to the querystring otherwise. (Although I'll also note I don't recall whether DevTools stats are collected client-side or server-side, and whether they're via unified analytics).

For DevTools, I'm wondering whether we should stop using* the querystring and instead have the server read these (and whatever DASH__TOOL becomes), so it has all of the information and can record the same things as other tools like the analysis server. I think ideally, all tools (DevTools server, Flutter, Pub, Analysis server) would know about these env vars, and report to a unified analytics consistently.

• And by stop using, I mean continue to pass it, but update the server to use the new env variables when set, but fall back to the querystring otherwise. (Although I'll also note I don't recall whether DevTools stats are collected client-side or server-side, and whether they're via unified analytics).

DevTools packages up the analytics event on the client and then sends it to unified analytics over DTD. So to read the environment variable from the DevTools server would be possible, but we would have to refactor how analytics dimensions are computed and add a new DevTools server API endpoint to retrieve the value.

The IDE and IDE feature are computed from the query string: https://github.com/flutter/devtools/blob/master/packages/devtools_app/lib/src/shared/analytics/_analytics_web.dart#L905

This is then used when creating the event for unified analytics: https://github.com/flutter/devtools/blob/master/packages/devtools_app/lib/src/shared/analytics/_analytics_web.dart#L905

And then sent to analytics over DTD here: https://github.com/flutter/devtools/blob/master/packages/devtools_app/lib/src/shared/analytics/_analytics_web.dart#L886-L887

Can you elaborate more on why you think the query string is not a good option? I'm not clear on what the risk is that would justify modifying how our analytics work, when just updating the query string would require no changes on the DevTools side.

@kenzieschmoll

Can you elaborate more on why you think the query string is not a good option? I'm not clear on what the risk is that would justify modifying how our analytics work, when just updating the query string would require no changes on the DevTools side.

I don't think it's a bad idea, I was just assuming you might want to capture the additional values discussed here (for example DASH__IDE_ENVIRONMENT might be useful to know when DevTools is running on a remote machine being proxied to the user if you're capturing things like load times) and if you were already capturing from the server, it might be easier to read from the env than define additional querystring variables. If either (or both) of those aren't true, it might not make sense to change that.

If all you want for DevTools is DASH__IDE_NAME in place of the existing value, that's an easy change (although I'll note that the value for VS Code is "Visual Studio Code" which is slightly different from the current value of "VSCode"). If you might want the other values too, we should probably document exactly the names we'll use (we could use the same as the env vars, or without the DASH__ prefix).

(and probably we should document this somewhere in the SDK once we've locked it down, since we want both/all IDEs to behave consistently, and there are multiple tools that may want to read the values)

I don't think it's a bad idea, I was just assuming you might want to capture the additional values discussed here (for example DASH__IDE_ENVIRONMENT might be useful to know when DevTools is running on a remote machine being proxied to the user if you're capturing things like load times) and if you were already capturing from the server, it might be easier to read from the env than define additional querystring variables. If either (or both) of those aren't true, it might not make sense to change that.

That makes sense. For now, I propose we just change the existing query parameter to read from DASH__IDE_NAME and then we can refactor our analytics to read from the server in the future if we want to collect more data with each DevTools event. GA does have restrictions on the number of dimensions that can be collected with each event, so we will have to weigh these data points against other dimensions we are collecting if we are close to the limit. CC @elliette please feel free weigh in if you disagree.

For now, I propose we just change the existing query parameter to read from DASH__IDE_NAME

Sure - I've filed #5970 about this and pinged you on it a few questions (so if it's not simple to just make the change now, we don't have to hold up landing this one).

Can you elaborate more on why you think the query string is not a good option?

The value of using environment variables is that they are automatically passed on to child processes, which is important for several reasons. And given that we're going to use environment variables everywhere else, it would be nice if DevTools could be consistent.

For now, I propose we just change the existing query parameter to read from DASH__IDE_NAME ...

This might be what you meant, but in the analysis server the plan is to read the environment variable if it exists, and to take the value from the command-line if it doesn't. That gives us reasonable semantics when there's a version skew.

Assuming no other feedback (or objections), I will merge this in the next few days.

(we can resolve the DevTools-related survey stuff via #5970, since it doesn't affect the values of these vars)