support truncating sourcemap lines #5033
Closed
Conversation
This file contains 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
stuartnelson3
commented
Mar 29, 2021
| @@ -240,6 +240,10 @@ apm-server: | |||
| # Sourcemapping is enabled by default. | |||
| #enabled: true | |||
|
|
|||
| # Maximum allowed line length in characters. Default maximum line length | |||
| # is unlimited, indicated by `0`. Minimum allowed value is 16. | |||
| #max_line_length: 0 | |||
There was a problem hiding this comment.
Are these options listed elsewhere and the apm-server.yml files generated? or do I add this to e.g. apm-server.docker.yml as well?
There was a problem hiding this comment.
Both apm-server.yml and apm-server.docker.yml are generated from _meta/beat.yml. Edit that one and run make update (or just make config).
stuartnelson3
commented
Mar 29, 2021
| Cache: &Cache{Expiration: defaultSourcemapCacheExpiration}, | ||
| IndexPattern: defaultSourcemapIndexPattern, | ||
| ESConfig: elasticsearch.DefaultConfig(), | ||
| MaxLineLength: 0, |
There was a problem hiding this comment.
0 indicates "no maximum"
There was a problem hiding this comment.
maybe add a doc comment to the struct field?
💔 Tests Failed
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪Test errors
Expand to view the tests failures> Show only the first 10 test failures
|
stuartnelson3
commented
Mar 29, 2021
sourcemap/mapper.go
Outdated
| const ( | ||
| sourcemapContentSnippetSize = 5 | ||
| // TODO: What value do we want here? | ||
| minMaxLineLength = 16 |
There was a problem hiding this comment.
I'm assuming 16 is much smaller than we want, it was just convenient for testing.
There was a problem hiding this comment.
What's the reason for having a minimum?
There was a problem hiding this comment.
Seems kind of silly if someone limits the output to 1 character, for example. But, I'm also happy to delete it.
stuartnelson3
commented
Mar 29, 2021
sourcemap/mapper.go
Outdated
|
|
||
| // limitScanLines is a modified form of bufio.ScanLines. It truncates any lines | ||
| // longer than the supplied `max` value. | ||
| func limitScanLines(max int) func([]byte, bool) (int, []byte, error) { |
There was a problem hiding this comment.
This is mostly copy and paste. I'm happy to remove this and just re-slice scanner.Text() above, but i figured if we're trying to avoid allocations, why not try to avoid allocations. (scanner.Text() allocates a new string)
There was a problem hiding this comment.
I don't think I understand how this would help. We're still calling scanner.Text() in Map above anyway, and slicing it shouldn't cause any more allocations?
I think I'd prefer the simpler option of slicing, but based on runes rather than bytes for consistency with similar limits elsewhere (namely in the agents, where most fields have a 1024 character/rune limit). There's some code that can be stolen for that here:
apm-server/processor/otel/consumer.go
Lines 810 to 820 in 141ee87
Longer term, perhaps after #5002 is implemented, I think we might want to split (and truncate) the source map content lines just once when parsing the source map, rather than every time we apply the source map.
There was a problem hiding this comment.
Yeah, it won't decrease the number of allocations, but I guess they'd potentially be smaller (which probably isn't much of a win).
|
This is a possible solution, but I'm happy to hear alternate suggestions. I like how this impl is using the api and setting I haven't tested this manually (or written a system test). Maybe I can steal someone for 15min tomorrow after standup and we can go through how to do this (upload a sourcemap and then view it in the UI). |
axw
reviewed
Mar 30, 2021
beater/config/rum.go
Outdated
Comment on lines
65
to
66
| // TODO: uint? uint64? Or do we not care and just use an int? | ||
| MaxLineLength uint `config:"max_line_length"` |
There was a problem hiding this comment.
not too fussed, but I'd probably go int since it's usually easier to work with and setting huge limits isn't sensible
| Cache: &Cache{Expiration: defaultSourcemapCacheExpiration}, | ||
| IndexPattern: defaultSourcemapIndexPattern, | ||
| ESConfig: elasticsearch.DefaultConfig(), | ||
| MaxLineLength: 0, |
There was a problem hiding this comment.
maybe add a doc comment to the struct field?
| @@ -240,6 +240,10 @@ apm-server: | |||
| # Sourcemapping is enabled by default. | |||
| #enabled: true | |||
|
|
|||
| # Maximum allowed line length in characters. Default maximum line length | |||
| # is unlimited, indicated by `0`. Minimum allowed value is 16. | |||
| #max_line_length: 0 | |||
There was a problem hiding this comment.
Both apm-server.yml and apm-server.docker.yml are generated from _meta/beat.yml. Edit that one and run make update (or just make config).
axw
reviewed
Mar 30, 2021
There was a problem hiding this comment.
Looks fine overall, though I'd prefer to simplify the truncation code.
I'm a little unclear on what triggered the OOM in the reported issue. Initially I thought it was about the length of lines in the source map itself, but in #4143 the author is only limiting the length of the source lines added to stack trace frames -- like you are here.
It would be good if you could try to reproduce the failure by indexing error events with very long source context lines. You might be able to simulate this by using the Go agent, passing in a custom stacktrace.ContextSetter to https://pkg.go.dev/go.elastic.co/apm#Tracer.SetContextSetter and capturing an error.
sourcemap/mapper.go
Outdated
|
|
||
| // limitScanLines is a modified form of bufio.ScanLines. It truncates any lines | ||
| // longer than the supplied `max` value. | ||
| func limitScanLines(max int) func([]byte, bool) (int, []byte, error) { |
There was a problem hiding this comment.
I don't think I understand how this would help. We're still calling scanner.Text() in Map above anyway, and slicing it shouldn't cause any more allocations?
I think I'd prefer the simpler option of slicing, but based on runes rather than bytes for consistency with similar limits elsewhere (namely in the agents, where most fields have a 1024 character/rune limit). There's some code that can be stolen for that here:
apm-server/processor/otel/consumer.go
Lines 810 to 820 in 141ee87
Longer term, perhaps after #5002 is implemented, I think we might want to split (and truncate) the source map content lines just once when parsing the source map, rather than every time we apply the source map.
sourcemap/mapper.go
Outdated
| const ( | ||
| sourcemapContentSnippetSize = 5 | ||
| // TODO: What value do we want here? | ||
| minMaxLineLength = 16 |
There was a problem hiding this comment.
What's the reason for having a minimum?
simitt
reviewed
Mar 30, 2021
sourcemap/mapper_test.go
Outdated
| assert.False(t, ok) | ||
| } | ||
|
|
||
| func TestMapNoMatch(t *testing.T) { | ||
| m, err := sourcemap.Parse("", []byte(test.ValidSourcemap)) | ||
| require.NoError(t, err) | ||
| assert.NoError(t, err) |
There was a problem hiding this comment.
why changing to assert? Running the tests if the parsing failed doesn't make much sense and would probably just lead to confusing errors (hiding the original issue). Similar for other changes below.
There was a problem hiding this comment.
ah sorry, just my OCD. I'll change them back
|
closing this for now. it doesn't solve the problem, and I haven't been able to reproduce the issue. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation/summary
User-requested feature to limit the line length for returned sourcemaps, as excessively long lines were OOM'ing coordinating nodes.
closes #4149
Checklist
For functional changes, consider:
How to test these changes
Start a copy of APM server with the
source_mapping.max_line_lengthset to something small. Confirm that when viewing lines, they've been truncated.Related issues