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

telemetry: Only send requests if data has changed #758

Merged
merged 1 commit into from
Jan 11, 2022
Merged

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Jan 10, 2022

Depends on hashicorp/terraform-schema#89
Depends on #761

According to manual testing with a single installed module from https://registry.terraform.io/modules/terraform-aws-modules/eks/aws/latest this can reduce the amount of telemetry requests to about half (from 32 to 14).

There's probably more we can do here - e.g. avoid sending telemetry for unsaved data. This is not only extra overhead, but also pollutes the data with "unfinished" provider or backend names. However this would require more effort - probably queueing the telemetry data somewhere and then adding a hook which gets triggered by textDocument/didSave. Either way not something I wanted to tackle in this PR.

@radeksimko radeksimko marked this pull request as ready for review January 11, 2022 10:44
@radeksimko radeksimko added the bug Something isn't working label Jan 11, 2022
@radeksimko radeksimko force-pushed the f-diff-telemetry branch 2 times, most recently from e643e06 to 40022da Compare January 11, 2022 11:17
@radeksimko radeksimko marked this pull request as draft January 11, 2022 11:19
@radeksimko radeksimko marked this pull request as ready for review January 11, 2022 11:27
@radeksimko radeksimko requested a review from a team January 11, 2022 12:09
Copy link
Member

@dbanck dbanck left a comment

Choose a reason for hiding this comment

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

Implementation LGTM! 🌟

I wondered if we prefer to send a "full" properties body when something changes or just the change? For example, we're sending providerRequirements and tfVersion in the current implementation, even if just tfRequirements has changed.

@radeksimko
Copy link
Member Author

I wondered if we prefer to send a "full" properties body when something changes or just the change? For example, we're sending providerRequirements and tfVersion in the current implementation, even if just tfRequirements has changed.

Yes, this is on purpose, because it makes querying the data in Kusto easier (more accurate) if we're querying against complete records.

@radeksimko radeksimko merged commit c2560e6 into main Jan 11, 2022
@radeksimko radeksimko deleted the f-diff-telemetry branch January 11, 2022 13:24
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants