Fix aggregate telemetry reliability #11134
Merged
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
ryzngard
commented
Oct 31, 2024
Co-authored-by: Dustin Campbell <[email protected]>
davidwengier
approved these changes
Oct 31, 2024
| { | ||
| public void Dispose() | ||
| { | ||
| ExportProvider.Dispose(); |
There was a problem hiding this comment.
A comment here for why this doesn't dispose TelemetryReporter, even though it is disposable, would be good.
Alternatively, TryGetTelemetryReporterAsync could just return the ExportProvider, and the caller could grab the telemetry reporter from it, and this type wouldn't be needed at all.
There was a problem hiding this comment.
That's true. It's only a tiny difference but I was trying to optimize for when CDK isn't installed so the provider wouldn't be needed to be kept.
ryzngard
commented
Nov 1, 2024
This was referenced Nov 7, 2024
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.
Currently telemetry is flushed after the language server host exits in VS Code. This is correctly flushing to the telemetry session, but can miss actually reporting the telemetry. In VS the lifetime of the TelemetrySession is managed centrally, but in VS Code it is up to each process to make sure it gets disposed properly. Without this telemetry that is fired could be dropped. This change addresses this by doing the following:
This should fix the reliability of some telemetry getting dropped. It still won't fix the scenario where the user force closes the process but better aligns VS and VS Code in expectation of lifetime management.
I noticed this while trying to set up dashboards. There are not nearly as many events as would be expected. If this still continues to be an issue one more option would be to flush on a timer. The data itself is aggregated and should handle that nicely.