Skip to content

Commit

Permalink
Keep original values when transforming. (elastic#647)
Browse files Browse the repository at this point in the history 
* Keep original values when transforming.

Persist original values when
* applying source mapping
* setting library_frame indicator
by storing original values within `original` namespace.

Ensure transform operation is idempotent,
by using original values if applied multiple times.
  • Loading branch information
@simitt
simitt committed Feb 21, 2018
1 parent ce3e36e commit d31160f
Show file tree
Hide file tree
Showing 8 changed files with 255 additions and 46 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.asciidoc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ https://github.com/elastic/apm-server/compare/71df0d96445df35afe27f38bcf734a0828
- Make `transaction.name` optional {pull}554[554]
- Remove config files from beats. Manually add relevant config options {pull}578[578]
- Use seperate index for uploaded `source maps` {pull}582[582].
- Store original values when applying source mapping or changing `library_frame` value {pull}647[647]

==== Deprecated

Expand Down
2 changes: 2 additions & 0 deletions docs/data/intake-api/generated/error/frontend.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,15 @@
"abs_path": "http://localhost:8000/test/../test/e2e/general-usecase/bundle.js.map",
"filename": "test/e2e/general-usecase/bundle.js.map",
"function": "<anonymous>",
"library_frame": true,
"lineno": 1,
"colno": 18
},
{
"abs_path": "http://localhost:8000/test/./e2e/general-usecase/bundle.js.map",
"filename": "~/test/e2e/general-usecase/bundle.js.map",
"function": "invokeTask",
"library_frame": false,
"lineno": 1,
"colno": 181
},
Expand Down
43 changes: 41 additions & 2 deletions model/stacktrace_frame.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,25 @@ type StacktraceFrame struct {
ExcludeFromGrouping bool

Sourcemap Sourcemap
Original Original
}

type Sourcemap struct {
Updated *bool
Error *string
}

type Original struct {
AbsPath *string
Filename string
Lineno int
Colno *int
Function *string
LibraryFrame *bool

sourcemapCopied bool
}

func (s *StacktraceFrame) Transform(config *pr.Config) common.MapStr {
enhancer := utility.NewMapStrEnhancer()
m := common.MapStr{}
Expand Down Expand Up @@ -67,6 +79,17 @@ func (s *StacktraceFrame) Transform(config *pr.Config) common.MapStr {
enhancer.Add(sm, "error", s.Sourcemap.Error)
enhancer.Add(m, "sourcemap", sm)

orig := common.MapStr{}
enhancer.Add(orig, "library_frame", s.Original.LibraryFrame)
if s.Sourcemap.Updated != nil && *(s.Sourcemap.Updated) {
enhancer.Add(orig, "filename", s.Original.Filename)
enhancer.Add(orig, "abs_path", s.Original.AbsPath)
enhancer.Add(orig, "function", s.Original.Function)
enhancer.Add(orig, "colno", s.Original.Colno)
enhancer.Add(orig, "lineno", s.Original.Lineno)
}
enhancer.Add(m, "original", orig)

return m
}

Expand All @@ -83,18 +106,21 @@ func (s *StacktraceFrame) setExcludeFromGrouping(pattern *regexp.Regexp) {
}

func (s *StacktraceFrame) setLibraryFrame(pattern *regexp.Regexp) {
s.Original.LibraryFrame = s.LibraryFrame
libraryFrame := pattern.MatchString(s.Filename) ||
(s.AbsPath != nil && pattern.MatchString(*s.AbsPath))
s.LibraryFrame = &libraryFrame
}

func (s *StacktraceFrame) applySourcemap(mapper sourcemap.Mapper, service Service, prevFunction string) string {
if s.Colno == nil {
s.setOriginalSourcemapData()

if s.Original.Colno == nil {
s.updateError("Colno mandatory for sourcemapping.")
return prevFunction
}
sourcemapId := s.buildSourcemapId(service)
mapping, err := mapper.Apply(sourcemapId, s.Lineno, *s.Colno)
mapping, err := mapper.Apply(sourcemapId, s.Original.Lineno, *s.Original.Colno)
if err != nil {
logp.NewLogger("stacktrace").Errorf("failed to apply sourcemap %s", err.Error())
e, isSourcemapError := err.(sourcemap.Error)
Expand All @@ -120,6 +146,19 @@ func (s *StacktraceFrame) applySourcemap(mapper sourcemap.Mapper, service Servic
return "<unknown>"
}

func (s *StacktraceFrame) setOriginalSourcemapData() {
if s.Original.sourcemapCopied {
return
}
s.Original.Colno = s.Colno
s.Original.AbsPath = s.AbsPath
s.Original.Function = s.Function
s.Original.Lineno = s.Lineno
s.Original.Filename = s.Filename

s.Original.sourcemapCopied = true
}

func (s *StacktraceFrame) buildSourcemapId(service Service) sourcemap.Id {
id := sourcemap.Id{ServiceName: service.Name}
if service.Version != nil {
Expand Down
138 changes: 94 additions & 44 deletions model/stacktrace_frame_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,34 @@ func TestApplySourcemap(t *testing.T) {
ver := "1"
service := Service{Name: "foo", Version: &ver}
for idx, test := range tests {
// check that original data are preserved,
// even when Transform function is applied twice.
if test.smapUpdated {
origAbsPath := *test.fr.AbsPath
origFilename := test.fr.Filename
origLineno := test.fr.Lineno
origColno := test.fr.Colno
origFunction := test.fr.Function

(&test.fr).applySourcemap(&FakeMapper{}, service, test.fct)
(&test.fr).applySourcemap(&FakeMapper{}, service, test.fct)

assert.Equal(t, origAbsPath, *test.fr.Original.AbsPath)
assert.Equal(t, origFilename, test.fr.Original.Filename)
assert.Equal(t, origLineno, test.fr.Original.Lineno)
if origColno == nil {
assert.Nil(t, test.fr.Original.Colno)
} else {
assert.Equal(t, *origColno, *test.fr.Original.Colno)
}
if origFunction == nil {
assert.Nil(t, test.fr.Original.Function)
} else {
assert.Equal(t, *origFunction, *test.fr.Original.Function)
}
}

// check that source mapping is applied as excpected
output := (&test.fr).applySourcemap(&FakeMapper{}, service, test.fct)
assert.Equal(t, test.outFct, output)
assert.Equal(t, test.lineno, test.fr.Lineno, fmt.Sprintf("Failed at idx %v; %s", idx, test.msg))
Expand Down Expand Up @@ -309,67 +337,89 @@ func TestLibraryFrame(t *testing.T) {
falsy := false
path := "/~/a/b"
tests := []struct {
fr StacktraceFrame
conf *pr.Config
libraryFrame *bool
msg string
fr StacktraceFrame
conf *pr.Config
libraryFrame *bool
origLibraryFrame *bool
msg string
}{
{fr: StacktraceFrame{},
conf: &pr.Config{},
libraryFrame: nil,
msg: "Empty StacktraceFrame, empty config"},
conf: &pr.Config{},
libraryFrame: nil,
origLibraryFrame: nil,
msg: "Empty StacktraceFrame, empty config"},
{fr: StacktraceFrame{AbsPath: &path},
conf: &pr.Config{LibraryPattern: nil},
libraryFrame: nil,
msg: "No pattern"},
conf: &pr.Config{LibraryPattern: nil},
libraryFrame: nil,
origLibraryFrame: nil,
msg: "No pattern"},
{fr: StacktraceFrame{AbsPath: &path},
conf: &pr.Config{LibraryPattern: regexp.MustCompile("")},
libraryFrame: &truthy,
msg: "Empty pattern"},
{fr: StacktraceFrame{},
conf: &pr.Config{LibraryPattern: regexp.MustCompile("~")},
libraryFrame: &falsy,
msg: "Empty StacktraceFrame"},
{fr: StacktraceFrame{AbsPath: &path},
conf: &pr.Config{LibraryPattern: regexp.MustCompile("^~/")},
libraryFrame: &falsy,
msg: "AbsPath given, no Match"},
{fr: StacktraceFrame{Filename: "myFile.js"},
conf: &pr.Config{LibraryPattern: regexp.MustCompile("^~/")},
libraryFrame: &falsy,
msg: "Filename given, no Match"},
conf: &pr.Config{LibraryPattern: regexp.MustCompile("")},
libraryFrame: &truthy,
origLibraryFrame: nil,
msg: "Empty pattern"},
{fr: StacktraceFrame{LibraryFrame: &falsy},
conf: &pr.Config{LibraryPattern: regexp.MustCompile("~")},
libraryFrame: &falsy,
origLibraryFrame: &falsy,
msg: "Empty StacktraceFrame"},
{fr: StacktraceFrame{AbsPath: &path, LibraryFrame: &truthy},
conf: &pr.Config{LibraryPattern: regexp.MustCompile("^~/")},
libraryFrame: &falsy,
origLibraryFrame: &truthy,
msg: "AbsPath given, no Match"},
{fr: StacktraceFrame{Filename: "myFile.js", LibraryFrame: &truthy},
conf: &pr.Config{LibraryPattern: regexp.MustCompile("^~/")},
libraryFrame: &falsy,
origLibraryFrame: &truthy,
msg: "Filename given, no Match"},
{fr: StacktraceFrame{AbsPath: &path, Filename: "myFile.js"},
conf: &pr.Config{LibraryPattern: regexp.MustCompile("^~/")},
libraryFrame: &falsy,
msg: "AbsPath and Filename given, no Match"},
conf: &pr.Config{LibraryPattern: regexp.MustCompile("^~/")},
libraryFrame: &falsy,
origLibraryFrame: nil,
msg: "AbsPath and Filename given, no Match"},
{fr: StacktraceFrame{Filename: "/tmp"},
conf: &pr.Config{LibraryPattern: regexp.MustCompile("/tmp")},
libraryFrame: &truthy,
msg: "Filename matching"},
{fr: StacktraceFrame{AbsPath: &path},
conf: &pr.Config{LibraryPattern: regexp.MustCompile("~/")},
libraryFrame: &truthy,
msg: "AbsPath matching"},
conf: &pr.Config{LibraryPattern: regexp.MustCompile("/tmp")},
libraryFrame: &truthy,
origLibraryFrame: nil,
msg: "Filename matching"},
{fr: StacktraceFrame{AbsPath: &path, LibraryFrame: &falsy},
conf: &pr.Config{LibraryPattern: regexp.MustCompile("~/")},
libraryFrame: &truthy,
origLibraryFrame: &falsy,
msg: "AbsPath matching"},
{fr: StacktraceFrame{AbsPath: &path, Filename: "/a/b/c"},
conf: &pr.Config{LibraryPattern: regexp.MustCompile("~/")},
libraryFrame: &truthy,
msg: "AbsPath matching, Filename not matching"},
conf: &pr.Config{LibraryPattern: regexp.MustCompile("~/")},
libraryFrame: &truthy,
origLibraryFrame: nil,
msg: "AbsPath matching, Filename not matching"},
{fr: StacktraceFrame{AbsPath: &path, Filename: "/a/b/c"},
conf: &pr.Config{LibraryPattern: regexp.MustCompile("/a/b/c")},
libraryFrame: &truthy,
msg: "AbsPath not matching, Filename matching"},
conf: &pr.Config{LibraryPattern: regexp.MustCompile("/a/b/c")},
libraryFrame: &truthy,
origLibraryFrame: nil,
msg: "AbsPath not matching, Filename matching"},
{fr: StacktraceFrame{AbsPath: &path, Filename: "~/a/b/c"},
conf: &pr.Config{LibraryPattern: regexp.MustCompile("~/")},
libraryFrame: &truthy,
msg: "AbsPath and Filename matching"},
conf: &pr.Config{LibraryPattern: regexp.MustCompile("~/")},
libraryFrame: &truthy,
origLibraryFrame: nil,
msg: "AbsPath and Filename matching"},
}

for _, test := range tests {
out := test.fr.Transform(test.conf)["library_frame"]
libFrame := test.fr.LibraryFrame
origLibFrame := test.fr.Original.LibraryFrame
if test.libraryFrame == nil {
assert.Nil(t, out, test.msg)
assert.Nil(t, libFrame, test.msg)
} else {
assert.Equal(t, *test.libraryFrame, out, test.msg)
assert.Equal(t, *test.libraryFrame, *libFrame, test.msg)
}
if test.origLibraryFrame == nil {
assert.Nil(t, origLibFrame, test.msg)
} else {
assert.Equal(t, *test.origLibraryFrame, *origLibFrame, test.msg)
}
}
}
Expand Down
22 changes: 22 additions & 0 deletions model/stacktrace_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,13 @@ func TestStacktraceTransformWithSourcemapping(t *testing.T) {
"line": common.MapStr{"column": 100, "number": 400},
"exclude_from_grouping": false,
"sourcemap": common.MapStr{"updated": true},
"original": common.MapStr{
"abs_path": "original path",
"colno": 1,
"filename": "original filename",
"function": "original function",
"lineno": 4,
},
},
{
"abs_path": "original path", "filename": "", "function": "original function",
Expand All @@ -177,19 +184,34 @@ func TestStacktraceTransformWithSourcemapping(t *testing.T) {
"line": common.MapStr{"column": 100, "number": 500},
"exclude_from_grouping": false,
"sourcemap": common.MapStr{"updated": true},
"original": common.MapStr{
"abs_path": "original path",
"colno": 1,
"filename": "original filename",
"function": "original function",
"lineno": 5,
},
},
{
"abs_path": "changed path", "filename": "changed filename", "function": "<anonymous>",
"line": common.MapStr{"column": 100, "number": 400},
"exclude_from_grouping": false,
"sourcemap": common.MapStr{"updated": true},
"original": common.MapStr{
"abs_path": "original path",
"colno": 1,
"filename": "/webpack",
"lineno": 4,
},
},
},
Msg: "Stacktrace with sourcemapping",
},
}

for idx, test := range tests {
// run `Stacktrace.Transform` twice to ensure method is idempotent
test.Stacktrace.Transform(&pr.Config{SmapMapper: &FakeMapper{}}, service)
output := test.Stacktrace.Transform(&pr.Config{SmapMapper: &FakeMapper{}}, service)
assert.Equal(t, test.Output, output, fmt.Sprintf("Failed at idx %v; %s", idx, test.Msg))
}
Expand Down
Loading

0 comments on commit d31160f

Please sign in to comment.