feat: Add sentry for serve.Serve function#54
Merged
yevgenypats merged 2 commits intomainfrom Sep 6, 2022
Merged
Conversation
plugin developers will be able to direct their plugin errors to their own projects on sentry as we dont want to monitor any community errors anyway. Also each plugin should go into it's own project on sentry because we are using server side grouping which are mostly project based.
afd8c5d to
247e07e
Compare
hermanschaaf
approved these changes
Sep 6, 2022
erezrokah
reviewed
Sep 6, 2022
Comment on lines
+225
to
+229
| // if pks[resource.PrimaryKeyValue()] { | ||
| // meta.Logger().Error().Str("table_name", t.Name).Str("primary_key", resource.PrimaryKeyValue()).Msg("duplicate primary key found") | ||
| // } else { | ||
| // pks[resource.PrimaryKeyValue()] = true | ||
| // } |
Member
There was a problem hiding this comment.
Suggested change
| // if pks[resource.PrimaryKeyValue()] { | |
| // meta.Logger().Error().Str("table_name", t.Name).Str("primary_key", resource.PrimaryKeyValue()).Msg("duplicate primary key found") | |
| // } else { | |
| // pks[resource.PrimaryKeyValue()] = true | |
| // } |
erezrokah
reviewed
Sep 6, 2022
| // we want to check for data integrity | ||
| // in the future we can do that as an optinoal feature via a flag | ||
| pks := map[string]bool{} | ||
| // pks := map[string]bool{} |
Member
There was a problem hiding this comment.
Suggested change
| // pks := map[string]bool{} |
erezrokah
reviewed
Sep 6, 2022
| cmd.Flags().StringVar(&network, "network", "tcp", `the network must be "tcp", "tcp4", "tcp6", "unix" or "unixpacket"`) | ||
| cmd.Flags().Var(logLevel, "log-level", fmt.Sprintf("log level. one of: %s", strings.Join(logLevel.Allowed, ","))) | ||
| cmd.Flags().Var(logFormat, "log-format", fmt.Sprintf("log format. one of: %s", strings.Join(logFormat.Allowed, ","))) | ||
| cmd.Flags().BoolVar(&noSentry, "no-sentry", false, "disable sentry") |
Member
There was a problem hiding this comment.
Maybe use the same wording as the CLI e.g. disable-telemetry?
Contributor
Author
There was a problem hiding this comment.
I think for flags it's better to use no- and I used it everywhere else (I don't think we have enable or at least shouldn't ) - this should make flags short. I can change the help to no sentry if this is more consistent.
Member
There was a problem hiding this comment.
Ah sure I don't mind no- vs disable- just not use an internal implementation detail in the name of the flag (if we can)
kodiakhq bot
pushed a commit
that referenced
this pull request
Sep 6, 2022
🤖 I have created a release *beep* *boop* --- ## [0.1.1](v0.1.0...v0.1.1) (2022-09-06) ### Features * Add custom faker ([#52](#52)) ([34bef4b](34bef4b)) * Add sentry for serve.Serve function ([#54](#54)) ([c1b508f](c1b508f)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
5 tasks
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
plugin developers will be able to direct their plugin errors
to their own projects on sentry as we dont want to monitor any
community errors anyway.
Also each plugin should go into it's own project on sentry
because we are using server side grouping which are mostly
project based.
Tested it on gcp - grouping works smoothly with nice title (no more diags.Error) for every single error as we use standard
sentry.Messageas well as using sentry mechanism to group and change title based on tags.Every error in the ETL process which is not ignore is sent with the error as string +
table_nameas tag.https://sentry.io/organizations/cloudquery-v2/issues/?project=6720365
hopefully this brings sane error monitoring one step closer :) 🚀
Use the following steps to ensure your PR is ready to be reviewed
go fmtto format your code 🖊golangci-lint run🚨 (install golangci-lint here)