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

ref: Use a Context type mapping to map[string]interface{} #444

Merged
merged 9 commits into from May 31, 2022
9 changes: 9 additions & 0 deletions .pre-commit-config.yaml
@@ -0,0 +1,9 @@
repos:
- repo: https://github.com/dnephin/pre-commit-golang
rev: 96221dc741cb30cc0136999083dc6bd0e2113000
hooks:
- id: go-build
- id: go-fmt
- id: go-imports
- id: golangci-lint
- id: no-go-testing
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this to a separate PR?

4 changes: 4 additions & 0 deletions CHANGELOG.md
@@ -1,5 +1,9 @@
# Changelog

## Unreleased

- ref: Use a Context type mapping to a map[string]interface{} for all contexts
phacops marked this conversation as resolved.
Show resolved Hide resolved

## v0.13.0

- ref: Change DSN ProjectID to be a string (#420)
Expand Down
10 changes: 5 additions & 5 deletions integrations.go
Expand Up @@ -72,31 +72,31 @@ func (ei *environmentIntegration) SetupOnce(client *Client) {
func (ei *environmentIntegration) processor(event *Event, hint *EventHint) *Event {
// Initialize maps as necessary.
if event.Contexts == nil {
event.Contexts = make(map[string]interface{})
event.Contexts = make(map[string]Context)
}
for _, name := range []string{"device", "os", "runtime"} {
if event.Contexts[name] == nil {
event.Contexts[name] = make(map[string]interface{})
event.Contexts[name] = make(Context)
}
}

// Set contextual information preserving existing data. For each context, if
// the existing value is not of type map[string]interface{}, then no
// additional information is added.
if deviceContext, ok := event.Contexts["device"].(map[string]interface{}); ok {
if deviceContext, ok := event.Contexts["device"]; ok {
if _, ok := deviceContext["arch"]; !ok {
deviceContext["arch"] = runtime.GOARCH
}
if _, ok := deviceContext["num_cpu"]; !ok {
deviceContext["num_cpu"] = runtime.NumCPU()
}
}
if osContext, ok := event.Contexts["os"].(map[string]interface{}); ok {
if osContext, ok := event.Contexts["os"]; ok {
if _, ok := osContext["name"]; !ok {
osContext["name"] = runtime.GOOS
}
}
if runtimeContext, ok := event.Contexts["runtime"].(map[string]interface{}); ok {
if runtimeContext, ok := event.Contexts["runtime"]; ok {
if _, ok := runtimeContext["name"]; !ok {
runtimeContext["name"] = "go"
}
Expand Down
14 changes: 7 additions & 7 deletions integrations_test.go
Expand Up @@ -357,13 +357,13 @@ func TestEnvironmentIntegrationDoesNotOverrideExistingContexts(t *testing.T) {
}
scope := NewScope()

scope.contexts["device"] = map[string]interface{}{
scope.contexts["device"] = Context{
"foo": "bar",
}
scope.contexts["os"] = map[string]interface{}{
scope.contexts["os"] = Context{
"name": "test",
}
scope.contexts["custom"] = "value"
scope.contexts["custom"] = Context{"key": "value"}
hub := NewHub(client, scope)
hub.CaptureMessage("test event")

Expand All @@ -378,13 +378,13 @@ func TestEnvironmentIntegrationDoesNotOverrideExistingContexts(t *testing.T) {

contexts := events[0].Contexts

if contexts["device"].(map[string]interface{})["foo"] != "bar" {
if contexts["device"]["foo"] != "bar" {
t.Errorf(`contexts["device"] = %#v, want contexts["device"]["foo"] == "bar"`, contexts["device"])
}
if contexts["os"].(map[string]interface{})["name"] != "test" {
if contexts["os"]["name"] != "test" {
t.Errorf(`contexts["os"] = %#v, want contexts["os"]["name"] == "test"`, contexts["os"])
}
if contexts["custom"] != "value" {
t.Errorf(`contexts["custom"] = %#v, want "value"`, contexts["custom"])
if contexts["custom"]["key"] != "value" {
t.Errorf(`contexts["custom"]["key"] = %#v, want "value"`, contexts["custom"]["key"])
}
}
6 changes: 4 additions & 2 deletions interfaces.go
Expand Up @@ -161,10 +161,12 @@ type Exception struct {
// An EventID must be 32 characters long, lowercase and not have any dashes.
type EventID string

type Context map[string]interface{}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we should introduce a concrete Context type. It'll force people to pass sentry.Context explicitly. Where if we'd use type alias, type Context = map[string]interface{}, it'd still work with most cases where people already pass a valid dictionary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I defined a new type since you mention on Slack it was a v0 and we should strive to make the API as it should. The type alias works too and the difference is minimal so that'll works well too.


// Event is the fundamental data structure that is sent to Sentry.
type Event struct {
Breadcrumbs []*Breadcrumb `json:"breadcrumbs,omitempty"`
Contexts map[string]interface{} `json:"contexts,omitempty"`
Contexts map[string]Context `json:"contexts,omitempty"`
Dist string `json:"dist,omitempty"`
Environment string `json:"environment,omitempty"`
EventID EventID `json:"event_id,omitempty"`
Expand Down Expand Up @@ -286,7 +288,7 @@ func (e *Event) transactionMarshalJSON() ([]byte, error) {
// NewEvent creates a new Event.
func NewEvent() *Event {
event := Event{
Contexts: make(map[string]interface{}),
Contexts: make(map[string]Context),
Extra: make(map[string]interface{}),
Tags: make(map[string]string),
Modules: make(map[string]string),
Expand Down
12 changes: 7 additions & 5 deletions interfaces_test.go
Expand Up @@ -126,8 +126,10 @@ func TestStructSnapshots(t *testing.T) {
Extra: map[string]interface{}{
"extra_key": "extra_val",
},
Contexts: map[string]interface{}{
"context_key": "context_val",
Contexts: map[string]Context{
"context_key": {
"context_key": "context_val",
},
},
},
},
Expand All @@ -138,14 +140,14 @@ func TestStructSnapshots(t *testing.T) {
Spans: []*Span{testSpan},
StartTime: time.Unix(3, 0).UTC(),
Timestamp: time.Unix(5, 0).UTC(),
Contexts: map[string]interface{}{
"trace": &TraceContext{
Contexts: map[string]Context{
"trace": TraceContext{
TraceID: TraceIDFromHex("90d57511038845dcb4164a70fc3a7fdb"),
SpanID: SpanIDFromHex("f7f3fd754a9040eb"),
Op: "http.GET",
Description: "description",
Status: SpanStatusOK,
},
}.Map(),
},
},
},
Expand Down
10 changes: 5 additions & 5 deletions scope.go
Expand Up @@ -28,7 +28,7 @@ type Scope struct {
breadcrumbs []*Breadcrumb
user User
tags map[string]string
contexts map[string]interface{}
contexts map[string]Context
extra map[string]interface{}
fingerprint []string
level Level
Expand All @@ -51,7 +51,7 @@ func NewScope() *Scope {
scope := Scope{
breadcrumbs: make([]*Breadcrumb, 0),
tags: make(map[string]string),
contexts: make(map[string]interface{}),
contexts: make(map[string]Context),
extra: make(map[string]interface{}),
fingerprint: make([]string, 0),
}
Expand Down Expand Up @@ -209,15 +209,15 @@ func (scope *Scope) RemoveTag(key string) {
}

// SetContext adds a context to the current scope.
func (scope *Scope) SetContext(key string, value interface{}) {
func (scope *Scope) SetContext(key string, value Context) {
scope.mu.Lock()
defer scope.mu.Unlock()

scope.contexts[key] = value
}

// SetContexts assigns multiple contexts to the current scope.
func (scope *Scope) SetContexts(contexts map[string]interface{}) {
func (scope *Scope) SetContexts(contexts map[string]Context) {
scope.mu.Lock()
defer scope.mu.Unlock()

Expand Down Expand Up @@ -358,7 +358,7 @@ func (scope *Scope) ApplyToEvent(event *Event, hint *EventHint) *Event {

if len(scope.contexts) > 0 {
if event.Contexts == nil {
event.Contexts = make(map[string]interface{})
event.Contexts = make(map[string]Context)
}

for key, value := range scope.contexts {
Expand Down
2 changes: 1 addition & 1 deletion scope_concurrency_test.go
Expand Up @@ -49,7 +49,7 @@ func TestConcurrentScopeUsage(t *testing.T) {

func touchScope(scope *sentry.Scope, x int) {
scope.SetTag("foo", "bar")
scope.SetContext("foo", "bar")
scope.SetContext("foo", sentry.Context{"foo": "bar"})
scope.SetExtra("foo", "bar")
scope.SetLevel(sentry.LevelDebug)
scope.SetTransaction("foo")
Expand Down