-
Notifications
You must be signed in to change notification settings - Fork 29
feat: add support for custom properties in error tracking #134
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
base: master
Are you sure you want to change the base?
Conversation
| ## 1.7.0 | ||
|
|
||
| **Breaking Changes:** | ||
| * `ExceptionInApi.Properties` type changed from `ExceptionInApiProperties` (struct) to `Properties` (map[string]interface{}) | ||
| * Removed `ExceptionInApiProperties` struct, replaced with `Properties` map for custom properties. | ||
| * Note: All existing system properties (`$lib`, `$lib_version`, `distinct_id`, `$exception_list`, etc.) are still attached and sent to the API, only the type representation has changed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we follow semver (Do we here?) this should be a major since it'll break people's apps compilation until they change the type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like in CHANGELOG.md we previously used a minor version for a breaking changes release, so not sure if we're strictly following semver. Happy to bump to 2.0.0 if we want to start being more strict about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, the versioning in Go got messed up at some point and people often release breaking changes as 1.X change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo, until the sdk reaches feature parity with the other more mature ones (e.g. js sdk), breaking changes are okay without major version bump. The APIs in go sdk still leaves a lot to be desired (compared to the other ones). I'd assume a few other breaking changes like this might be necessary going forward.
error_tracking.go
Outdated
| func NewDefaultException( | ||
| timestamp time.Time, | ||
| distinctID, title, description string, | ||
| properties ...Properties, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is confusing and will lead to errors.
It's know sometimes people abuse variadic arg number in Go, but I think we should just proper properties Properties, that's all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alternatively, add a method to Exception: WithProperties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea, I'll do WithProperties instead.
| func (msg Exception) APIfy() APIMessage { | ||
| libVersion := getVersion() | ||
|
|
||
| properties := Properties{}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, wouldn't it be better to re-use msg.Properties instead of creating a new one here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't do that unfortunately, because that'd mutate the caller's msg.Properties (preferably no side-effects happen), so we create a new one instead
| Fallback: "<no linked error>", | ||
| }, | ||
| properties: func(ctx context.Context, r slog.Record) Properties { | ||
| return NewProperties() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, but is it worth considering having something like this (shamelessly taken from the unit test) as default behavior?
| return NewProperties() | |
| props := NewProperties() | |
| r.Attrs(func(a slog.Attr) bool { | |
| props.Set(a.Key, a.Value.Any()) | |
| return true | |
| }) | |
| return props |
I figure that most users will likely do something like this as their default behaviour, and if they don't they can override it through WithPropertiesFn.
Closes #132
Exceptions now support custom properties. Technically backwards compatible since
NewDefaultExceptioncan be called w/o properties, but since we dropExceptionInApiPropertiesit's a breaking change. Also added slog support.