Use Telemetry v1.75 VSCode API - #4457

@DanTup Fixed some of the things on your comments

#4377

I think some properties on the internal google analytics class need to be moved back there because the send method needs them like flutter sdk version, is using dap or lsp

@aitor-gomila

I think some properties on the internal google analytics class need to be moved back there because the send method needs them like flutter sdk version, is using dap or lsp

Do you mean move them to the TelemetrySender? If so, why can't we keep them on Analytics? I'd prefer the sender class to be only doing the web request, and the Analytics class to add in all other information. That way what the user sees if they look at the analytics logs is more accurate and there isn't extra info being added in after they see it.

@aitor-gomila I had a quick look through the analytics and here's a list of things I'd like to remove. I can do this on master but it may cause some unnecessary conflicts for you, so you're welcome to remove them here (or I can remove them later).

• all time() calls - I don't think these timings are providing a lot of info, and it'll simplify the API slightly
• logExtensionShutdown - there's not much value in this (it should always be the same number as the extension activates, except where the shutdown will abort it being recorded, so not much value)
• logError (as previously discussed)
• logDebuggerRestart & logDebuggerHotReload (these generate a lot of events and were originally to get an idea of whether people knew about hot restart/reload, which are now much more integrated and somewhat obvious)

Sorry for my terrible explanation.

The send method sends whatever you tell it to, plus some other device info (e.g. whether the user is using DAP or LSP).

GoogleAnalyticsTelemetrySender is merely a TelemetrySender implementation that is turned into a TelemetryLogger so Analytics can only call the methods and fields TelemetryLogger defines. In this case, logUsage, logError, and flush.

The problem is GoogleAnalyticsTelemetrySender needs to be able to access to workspaceContext and a few other things in order to have all the necessary information, and this is done by including workspaceContext and other things into the constructor.

So Analytics would just be a fancier way to call the TelemetryLogger.

Is that correct? Any feedback?

Just one more thing: I think one of the calls to this.event() which is now this.telemetryLogger.logUsage returned something. With TelemetryLogger this is no longer possible. Need to check what it does again.

@aitor-gomila I had a quick look through the analytics and here's a list of things I'd like to remove. I can do this on master but it may cause some unnecessary conflicts for you, so you're welcome to remove them here (or I can remove them later).

* all `time()` calls - I don't think these timings are providing a lot of info, and it'll simplify the API slightly

* `logExtensionShutdown` - there's not much value in this (it should always be the same number as the extension activates, except where the shutdown will abort it being recorded, so not much value)

* `logError` (as previously discussed)

* `logDebuggerRestart` & `logDebuggerHotReload` (these generate a lot of events and were originally to get an idea of whether people knew about hot restart/reload, which are now much more integrated and somewhat obvious)

Agreed, these cause unnecessary complexity, but I didn't want to change intended behaviour. logError I think needs to be kept but we can return; and call it a day. Flush can be removed safely and I intended to do so.

The problem is GoogleAnalyticsTelemetrySender needs to be able to access to workspaceContext and a few other things in order to have all the necessary information, and this is done by including workspaceContext and other things into the constructor.

This information is just added to custom dimensions though, so it should be possible to just include it in the data passed to logUsage?

Right now (before your changes), there's an event method in Analytics that just makes an object with all the data (including the SDK versions, etc.). I think we can still do that in Analytics, and have the GoogleAnalyticsTelemetrySender just receive that whole data and send it, without needing to add anything more to it?

logError I think needs to be kept but we can return; and call it a day. Flush can be removed safely and I intended to do so.

Sorry, my comment was ambiguous here :) I meant the logError in our Analytics class can be removed (and any calls to it). You're right that the one in the sender may need to stay (and can be empty).

This information is just added to custom dimensions though, so it should be possible to just include it in the data passed to logUsage?

Ok, giving it a thought, it makes sense to separate the concerns of Analytics and the Sender.

Ok @DanTup, updated some of the code.

• Restored event method in Analytics class.
• Return softly in sendErrorData.
• Simplify sendEventData method in GoogleAnalyticsTelemetrySender.
• Remove Analytics.time calls.
• Remove handleError and logError in Analytics.

Since Analytics.event() now returns void instead of Promise<void>, some of the code is not working properly.

Also, some portions of GoogleAnalyticsTelemetrySender.send require a logger, I think it's fair to provide one in this case? I think we should pass those callbacks up and let Analytics handle it

Things to fix:

• send method uses logger and handleError.
• calling this.event doesn't return a promise anymore

Don't tell me how to do these, I want to learn problem solving myself. But PR is almost 99% done. Needs some testing before merging, though

Don't tell me how to do these, I want to learn problem solving myself

Sure! Let me know when you want me to take a look and make comments.

Thanks for your work on this so far! :-)

I've got a question. Send uses a handleError function. But since it's a promise, why not reject and let Analytics handle it?

In this line I'm getting a TypeError, but you are not:

req.write(querystring.stringify(data));

data is any, so it doesn't accept it, but you are using any too and it works? Is it because you are using Object.assign?

I've got a question. Send uses a handleError function. But since it's a promise, why not reject and let Analytics handle it?

I think you're right, we could change:

this.handleError(e);
resolve();

to:

reject(e);

and then wrap a try/catch around the code starting await new Promise<void>( and instead call handleError in a catch block.

It's been a long time since I wrote this so I'm not sure if there was a reason it was done this way, but you're welcome to change it as long as we ensure we're handling the error here (and not relying on the caller to catch it).

data is any, so it doesn't accept it, but you are using any too and it works? Is it because you are using Object.assign?

In the original code, data is not any inside send. customData is any, but data's type is inferred from the object literal where it is defined:

We should probably avoid using explicit anys where possible (TS complains about it more than it used to). Perhaps Record<String, unknown> or Record<String, any> would work for data's type?

Why not use .catch(logger.info) instead of try/catch?

I think either will work. I generally prefer using await/try/catch rather than .then()/.catch() because I think the resulting code is usually easier to read (it's more like non-promise code), but sometimes I use the latter. Whichever seems to fit best for the code you're writing is fine (generally performance isn't a real concern here.. we should be sending analytics so infrequently that the time will be negligible, and hopefully exceptions are even less common).

Just one more thing: do you use ESLint/Prettier?

ES Lint should be set up. You can run npm run lint, but it should just show up in VS Code too (you might need the dbaeumer.vscode-eslint extension which is noted in .vscode/extensions.json).

except try/catch is synchronous code which defeats the purpose of Promises. Await too. If you don't use this frequently it's okay. But try to avoid it.

But do whatever. Not trying to sound nitpicky :)

Now I need to re-introduce that large object const data = {...}

Everything seems to be ready. Make sure to test it well because I have not tried any of these changes If you need me to do any changes let me know ;)

except try/catch is synchronous code which defeats the purpose of Promises. await too.

This isn't accurate :) Using await/try/catch with promises is still async code, it's just the same as using .then() and .catch(). The code is still asynchronous at awaits. I (generally) prefer it because it reduces the differences between async/sync code - you can use the same pattern (try/catch) for handling your errors.

I'll try out the changes and add feedback shortly - thanks again!

@aitor-gomila this all looks great! I can see the analytics being sent, and they show up in the new VS Code output pane ("Extension Telemetry") when log level is set to trace:

Thank you for the contribution!

:,)

We should have a more visual way for thw user to see them, though!

VSCode's fault most likely tho.

Its nice to see this was born out of a confusion.

We should have a more visual way for thw user to see them, though!

That would be nice, but I think it's a bit out of scope for the extension - it would be better if something like that was handled by VS Code (since it should apply to all extensions and not be something specific to Dart). Maybe it's something they'll consider if everyone adopts this new unified API :-)