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.

Sign up for GitHub

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

Jump to bottom

Keep original values when transforming. #647

Merged
merged 3 commits into from
Feb 8, 2018
Merged

Keep original values when transforming. #647

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
447cefe
Keep original values when transforming.
simitt Feb 7, 2018
2ba8855
run make update.
simitt Feb 7, 2018
9733106
Add Changelog.
simitt Feb 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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