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: Send telemetry about failed COPY FROMs #1050

Merged
merged 4 commits into from
Jul 7, 2022
Merged

feat: Send telemetry about failed COPY FROMs #1050

merged 4 commits into from
Jul 7, 2022

Conversation

irmatov
Copy link
Contributor

@irmatov irmatov commented Jun 30, 2022

Closes cloudquery/cloudquery-issues#374

@irmatov irmatov self-assigned this Jun 30, 2022
@irmatov
Copy link
Contributor Author

irmatov commented Jun 30, 2022

Depends on new SDK with cloudquery/cq-provider-sdk#395

@irmatov irmatov requested review from roneli and disq June 30, 2022 14:51
@irmatov irmatov marked this pull request as ready for review June 30, 2022 14:51
@irmatov irmatov requested a review from a team as a code owner June 30, 2022 14:51
@erezrokah
Copy link
Contributor

@irmatov looks like you need to update a test for CI to pass https://github.com/cloudquery/cloudquery/runs/7177084872?check_suite_focus=true#step:8:269

Closes cloudquery/cloudquery-issues#374
@irmatov
Copy link
Contributor Author

irmatov commented Jul 5, 2022

@erezrokah looks like a flaky test. it worked after rebase & push.

cmd/fetch/fetch.go Outdated Show resolved Hide resolved
@@ -162,7 +160,7 @@ func GetCookieId() uuid.UUID {
return id
}

func Capture(eventType string, providers registry.Providers, data Message, diags diag.Diagnostics, extra ...interface{}) {
func Capture(eventType string, providers registry.Providers, data Message, extra ...interface{}) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not keep diags param as-is but pass in summarized versions? Repeating the "diagnostics" string (and remembering to set success) as extra is prone to errors (and forgetting). If you don't have any (sendProviderTelemetryEvents) could just pass nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't pass summarized version as it still would introduce core import into the analytics and create an import loop. Moved SummarizeDiagnostics into the analytics package.

@@ -0,0 +1,39 @@
package analytics
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved from core without any changes

@@ -40,32 +32,6 @@ func (d *SentryDiagnostic) IsSentryDiagnostic() (bool, map[string]string, bool)
return true, d.Tags, d.Ignore
}

func SummarizeDiagnostics(diags diag.Diagnostics) DiagnosticsSummary {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved into analytics package to break import cycle.

cmd/fetch/fetch.go Outdated Show resolved Hide resolved
pkg/core/fetch.go Outdated Show resolved Hide resolved
Copy link
Member

@disq disq left a comment

Choose a reason for hiding this comment

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

LGTM with nits :)

@irmatov irmatov merged commit 1da0478 into cloudquery:main Jul 7, 2022
@irmatov irmatov deleted the feat/copy_from_analytics branch July 7, 2022 06:34
erezrokah pushed a commit that referenced this pull request Aug 14, 2022
Synced local file(s) with [cloudquery/.github](https://github.com/cloudquery/.github).





---

This PR was created automatically by the [repo-file-sync-action](https://github.com/BetaHuhn/repo-file-sync-action) workflow run [#2489093639](https://github.com/cloudquery/.github/actions/runs/2489093639)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants