Skip to content
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

feat: Telemetry reporting #187

Merged
merged 29 commits into from
Nov 1, 2021
Merged

feat: Telemetry reporting #187

merged 29 commits into from
Nov 1, 2021

Conversation

michalbali256
Copy link
Contributor

No description provided.

@michalbali256 michalbali256 marked this pull request as ready for review October 11, 2021 09:31
Copy link
Contributor

@slavek-kucera slavek-kucera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the code smells should probably be addressed.

.github/workflows/CI.yml Outdated Show resolved Hide resolved
clients/vscode-hlasmplugin/README.md Outdated Show resolved Hide resolved
clients/vscode-hlasmplugin/README.md Outdated Show resolved Hide resolved
clients/vscode-hlasmplugin/src/extension.ts Show resolved Hide resolved

// setTimeout is needed, because telemetry initialization is asynchronous
// and AFAIK no event in the API is exposed to send the activation telemetry event
setTimeout(() => {telemetry.reportEvent("hlasm.activated", {server_variant:serverVariant.toString()});}, 1000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it be triggered by e.g. the LanguageClient onReady event?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. I wanted to have an event that would trigger right after activation and would not depend on e.g. ability to run native executables.

BTW creators of vscode-extension-telemetry acknowledged that this is a shortfall of the plugin and plan to either introduce an event or buffer where such event would be stored before they can be sent. microsoft/vscode-extension-telemetry#76

language_server/src/dap/dap_feature.cpp Outdated Show resolved Hide resolved
clients/vscode-hlasmplugin/src/extension.ts Outdated Show resolved Hide resolved
language_server/src/dap/dap_server.cpp Outdated Show resolved Hide resolved
Comment on lines 77 to 83
struct telemetry_metrics_info
{
json properties;
json metrics;
};

telemetry_metrics_info virtual get_telemetry_details();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a bit strange... how is the method supposed to know to which request the details are being provided?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it is a bit strange... the details are provided for the last request.

It fetches the details for requests with telemetry_log_level::LOG_WITH_METRICS, which is actually only one request (didOpen).

I only wanted the lsp_server to have the metadata consumer, but at the same time I wanted to log the requests in the server::call_method, method, which is common for lsp and dap server. Thats why I ended up with this virtual method.

language_server/src/lsp/lsp_server.h Outdated Show resolved Hide resolved
@zimlu02 zimlu02 added this to the 0.15.0 milestone Oct 14, 2021
return this.telemetry_key;
}

private getTelemetryResourcePath() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not used anymore?

return vscode.extensions.getExtension(EXTENSION_ID).packageJSON.version;
}

private getExtensionPath(): string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only used in getTelemetryResourcePath?

Comment on lines 176 to 177
// void server::write(const nlohmann::json& payload) { notify("telemetry/event", payload); }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// void server::write(const nlohmann::json& payload) { notify("telemetry/event", payload); }


namespace hlasm_plugin::language_server {

struct parsing_metadata_collector : public parser_library::parsing_metadata_consumer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
struct parsing_metadata_collector : public parser_library::parsing_metadata_consumer
struct parsing_metadata_collector final : public parser_library::parsing_metadata_consumer

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 9 Code Smells

74.8% 74.8% Coverage
0.0% 0.0% Duplication

@michalbali256 michalbali256 merged commit 70445dd into development Nov 1, 2021
@github-actions
Copy link

github-actions bot commented Nov 3, 2021

🎉 This PR is included in version 0.15.0-beta.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@github-actions
Copy link

github-actions bot commented Nov 8, 2021

🎉 This PR is included in version 0.15.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants