Add support for sourcemap uploads #302

jalvz · 2017-11-14T19:26:31Z

No description provided.

jalvz · 2017-11-17T15:27:27Z

@roncohen this is ready for review!

roncohen · 2017-11-20T16:45:23Z

Nice @jalvz!

A couple comments:

We discussed this briefly earlier, but how would you feel about letting the sourcemap library handle validation of the uploaded sourcemap instead of doing it in JSON Schema? We can still use JSON Schema to validate the other fields (app_name, app_version), but just let the sourcemap itself be validated as a string and then make the sourcemap validator validate by passing the sourcemap to the library. There might be other things than just the JSON structure that it needs to validate and we can't validate those with JSON Schema anyway. Just an idea. EDIT: I think the overhead of the library parsing the sourcemap again is OK.
Did you check how the sourcemap is stored in ES? We need to make sure it's not being indexed.
TestProcessSourcemapNullValues.approved.json and TestProcessSourcemapMinimalPayload.approved.json look the same to me. Do you know what the difference is? Maybe we can skip some of those tests?
Error messages given by the JSON validation for sourcemaps will refer to field names that are unknown to the user (app.name vs app-name, bundle-filepath vs. bundle_filepath etc.). We have to align the map we create here so it has the same names as what the users puts in, so that the error messages refer to the same fields.

jalvz · 2017-11-21T15:18:44Z

re. handling validation with parsing library:
not sure what you have in mind, like this c319f55?

re. approved files: they look the same because tests ensure that passing attributes with null values is equivalent to not passing attributes.

jalvz · 2017-11-22T14:50:01Z

4th point above implemented here d4ab32d

roncohen · 2017-11-23T10:41:51Z

beater/handlers.go

@@ -36,6 +37,7 @@ const (
 	BackendErrorsURL        = "/v1/errors"
 	FrontendErrorsURL       = "/v1/client-side/errors"
 	HealthCheckURL          = "/healthcheck"
+	SourcemapsURL           = "/sourcemaps"


This is not versioned. It might be fine, but could you explain the rationale briefly?

roncohen · 2017-11-23T10:42:49Z

_meta/beat.yml

@@ -7,7 +7,7 @@ apm-server:
  # Defines the host and port the server is listening on
  host: "localhost:8200"

-  frontend.enabled: false
+  frontend.enabled: true


I think this would enable frontend endpoints by default. I suggest we keep it off by default for now and reevaluate once we have sourcemap processing of errors in.

roncohen · 2017-11-23T10:50:20Z

processor/sourcemap/_meta/fields.yml

+              type: keyword
+              description: >
+                App version.
+


Tiny thing: Could we put the sourcemap field in here so it shows up in the docs but it will still not get indexed?

I don't know. How do you do that? I can't find anywhere else where we do that.

No that's not possible, see #28

roncohen · 2017-11-23T10:50:46Z

Good work @jalvz! Getting close 👍

simitt

Can you please add a changelog entry.
Otherwise LGTM

roncohen · 2017-11-27T15:45:14Z

Cool @jalvz. Could you squash?

jalvz · 2017-11-27T16:21:18Z

done. i wouldnt like to squash more than this, but if you are not happy with it feel free to squash and merge yourself.

roncohen · 2017-11-28T09:04:54Z

script/output_data/output_data.go

@@ -28,7 +28,7 @@ func generate() error {

 	for path, mapping := range beater.Routes {

-		if path == beater.HealthCheckURL {
+		if path == beater.HealthCheckURL || path == beater.SourcemapsURL {


Is there still a reason for skipping sourcemaps here?

What do you think about adding sourcemaps to a docs/generated/sourcemaps folder, and not to the intake-api ? So we would have it documented but clearly distinguished from standard intake API.
I suggest to merge this and add the doc script in a seperate PR.

no, updated

@roncohen

Refactor based on suggestions from @roncohen * Move sourcemap decoding to the sourcemap package * Change the way to build the payload * Make the decoder return a buffer

@roncohen

…essage as it is input'ed by the user, as per @roncohen code review. This requires to flatten the app attributes, so they are no longer reusable. This reverts commit b7174c2. generate ES output for sourcemap

roncohen · 2017-11-29T09:45:31Z

awesome! Great work @jalvz !

jalvz self-assigned this Nov 14, 2017

jalvz added the in progress label Nov 14, 2017

jalvz force-pushed the sourcemap-upload branch 6 times, most recently from e85b55c to 4d6ccc4 Compare November 17, 2017 15:26

jalvz added review and removed in progress labels Nov 17, 2017

jalvz force-pushed the sourcemap-upload branch from cb2709b to 1ce61cd Compare November 21, 2017 13:53

roncohen reviewed Nov 23, 2017

View reviewed changes

simitt approved these changes Nov 24, 2017

View reviewed changes

jalvz force-pushed the sourcemap-upload branch from b6e4cf2 to e3161eb Compare November 24, 2017 15:50

jalvz force-pushed the sourcemap-upload branch from e3161eb to 603d16e Compare November 27, 2017 16:14

roncohen reviewed Nov 28, 2017

View reviewed changes

Juan A added 5 commits November 28, 2017 14:12

Support for sourcemap uploads

f5c3a66

Refactor based on suggestions from @roncohen * Move sourcemap decoding to the sourcemap package * Change the way to build the payload * Make the decoder return a buffer

Added test cases

cfa60f7

validate sourcemaps with the parsing library as per code review comment

569e652

Make jsonschema validation return the exact field name in the error m…

26d9566

…essage as it is input'ed by the user, as per @roncohen code review. This requires to flatten the app attributes, so they are no longer reusable. This reverts commit b7174c2. generate ES output for sourcemap

update changelog

83410e1

jalvz force-pushed the sourcemap-upload branch from dc58209 to 83410e1 Compare November 28, 2017 13:13

simitt merged commit e0d7999 into elastic:master Nov 28, 2017

jalvz deleted the sourcemap-upload branch February 1, 2018 09:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for sourcemap uploads #302

Add support for sourcemap uploads #302

jalvz commented Nov 14, 2017

jalvz commented Nov 17, 2017

roncohen commented Nov 20, 2017 •

edited

Loading

jalvz commented Nov 21, 2017

jalvz commented Nov 22, 2017 •

edited

Loading

roncohen Nov 23, 2017

roncohen Nov 23, 2017 •

edited

Loading

roncohen Nov 23, 2017

jalvz Nov 23, 2017

simitt Nov 24, 2017

roncohen Nov 24, 2017

roncohen commented Nov 23, 2017

simitt left a comment •

edited

Loading

roncohen commented Nov 27, 2017

jalvz commented Nov 27, 2017

roncohen Nov 28, 2017

simitt Nov 28, 2017

jalvz Nov 28, 2017

roncohen commented Nov 29, 2017

Add support for sourcemap uploads #302

Add support for sourcemap uploads #302

Conversation

jalvz commented Nov 14, 2017

jalvz commented Nov 17, 2017

roncohen commented Nov 20, 2017 • edited Loading

jalvz commented Nov 21, 2017

jalvz commented Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

roncohen Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roncohen commented Nov 23, 2017

simitt left a comment • edited Loading

Choose a reason for hiding this comment

roncohen commented Nov 27, 2017

jalvz commented Nov 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roncohen commented Nov 29, 2017

roncohen commented Nov 20, 2017 •

edited

Loading

jalvz commented Nov 22, 2017 •

edited

Loading

roncohen Nov 23, 2017 •

edited

Loading

simitt left a comment •

edited

Loading