Skip to content

fix: Send resource validation errors to Sentry#601

Merged
kodiakhq[bot] merged 4 commits intomainfrom
sentry-resource-validation-errors
Jan 11, 2023
Merged

fix: Send resource validation errors to Sentry#601
kodiakhq[bot] merged 4 commits intomainfrom
sentry-resource-validation-errors

Conversation

@hermanschaaf
Copy link
Copy Markdown
Contributor

@hermanschaaf hermanschaaf commented Jan 11, 2023

Resource validation errors, like a row missing a PK value, are non-recoverable and need to be fixed by the plugin author. As such, we should know about them when they happen, even though they do not cause a panic in the code. I tried to minimize the number of duplicate messages we send in this case by using a sync.Map

@github-actions github-actions bot added fix and removed fix labels Jan 11, 2023
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 11, 2023

⏱️ Benchmark results

  • DefaultConcurrencyDFS-2 resources/s: 11,010
  • DefaultConcurrencyRoundRobin-2 resources/s: 11,907
  • Glob-2 ns/op: 144.8
  • TablesWithChildrenDFS-2 resources/s: 30,458
  • TablesWithChildrenRoundRobin-2 resources/s: 30,500
  • TablesWithRateLimitingDFS-2 resources/s: 28.45
  • TablesWithRateLimitingRoundRobin-2 resources/s: 787.5
  • BufferedScanner-2 ns/op: 9.396
  • LogReader-2 ns/op: 30.46

if err := resolvedResource.Validate(); err != nil {
tableMetrics := p.metrics.TableClient[table.Name][client.ID()]
p.logger.Error().Err(err).Str("table", table.Name).Str("client", client.ID()).Msg("resource resolver finished with validation error")
if _, found := sentValidationErrors.LoadOrStore(table.Name, struct{}{}); !found {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we we want only to send once per table? I guess it makes sense even though we can miss some things but yeah let's start with that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we may miss some things, but I hope not. And we'll catch it once the first issue is fixed anyway. I figure it's better than using our entire Sentry quota in one sync (unless I'm misunderstanding how Sentry billing works)

@kodiakhq kodiakhq bot merged commit 5916516 into main Jan 11, 2023
@kodiakhq kodiakhq bot deleted the sentry-resource-validation-errors branch January 11, 2023 16:38
kodiakhq bot pushed a commit that referenced this pull request Jan 12, 2023
🤖 I have created a release *beep* *boop*
---


## [1.25.0](v1.24.2...v1.25.0) (2023-01-11)


### Features

* **docs:** Sort tables ([#599](#599)) ([8a3bfad](8a3bfad))
* **transformers:** Add support for `net.IP` ([#595](#595)) ([a420645](a420645))
* **transformers:** Add WithPrimaryKeys option ([#598](#598)) ([107006c](107006c))


### Bug Fixes

* Send resource validation errors to Sentry ([#601](#601)) ([5916516](5916516))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
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