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

index the url path only of the sourcemaps #1661

Merged
merged 2 commits into from Jan 16, 2019

Conversation

@jalvz
Copy link
Collaborator

jalvz commented Dec 17, 2018

fixes #1649

this is the best i came up with, as wildcard queries are slow in elasticsearch (particularly if the wildcard is at the beginning, like in here) and caching would be quite complicated too

  • needs changelog
Copy link
Collaborator

simitt left a comment

I assume there are valid use cases where one would want to upload two different sourcemaps with the same path, e.g. "http://service1/general-usecase/bundle.js.map" and "http://service2/general-usecase/bundle.js.map". If I am not mistaken, then for such use cases this would be a breaking change, as the query for the source map would return any matching sourcemap for the path and store it with the path in the cache.

I suggest to add a config option to opt in for only using url path.

@jalvz

This comment has been minimized.

Copy link
Collaborator Author

jalvz commented Dec 17, 2018

in that case the services would be different, so they wouldn't match, right? is there a realistic case for same service, version and url path, but different hostname?

if so, i also could also add a boost to the full url query term and sort by relevance. either case for me it feels like overdoing it, but i dont know.

@simitt

This comment has been minimized.

Copy link
Collaborator

simitt commented Dec 17, 2018

Right, I didn't think of the service name being sent up. I'll give it some more thought.

@simitt

This comment has been minimized.

Copy link
Collaborator

simitt commented Dec 18, 2018

The implementation LGTM, thanks for taking care of this @jalvz !
From a use-case point of view, I am not sure if this can cause troubles, @jahtalab could you give this change a thought from a users perspective?

@jahtalab

This comment has been minimized.

Copy link
Contributor

jahtalab commented Dec 18, 2018

Just had a zoom call with @jalvz , decided to consider searching for an exact match first and if that fails to search for only the path part!

@jahtalab

This comment has been minimized.

Copy link
Contributor

jahtalab commented Dec 18, 2018

@jalvz , I was just thinking about it, what about splitting the path to its parts during the upload process and saving those parts with the sourcemap?
We can then only search for the path but after having all the results we can find the best match and even maybe support wildcards on the apm-server!

I'm just thinking out loud here, but might be worth investigating that as well 😄

@jalvz

This comment has been minimized.

Copy link
Collaborator Author

jalvz commented Dec 18, 2018

thanks @jahtalab!
The straightforward way would be 6401bb8, but we could investigate other alternatives indeed.

@jalvz jalvz force-pushed the jalvz:index-sourcemap-path-only branch from 6401bb8 to e294e01 Dec 18, 2018
@jalvz jalvz force-pushed the jalvz:index-sourcemap-path-only branch from e294e01 to 13ec5ce Jan 7, 2019
Copy link
Collaborator

simitt left a comment

I am no expert with use cases for sourcemaps, but this generally looks sane to me (apart from the comment I left).

// backwards compatibility
m.Accessor.Remove(id)

id.Path = utility.UrlPath(id.Path)

This comment has been minimized.

Copy link
@simitt

simitt Jan 7, 2019

Collaborator

Afaics this line needs to be removed again, otherwise the two term queries in https://github.com/elastic/apm-server/pull/1661/files#diff-5ea9ad2b2641661a7ac259166eb8142dR114 are the same.

@jalvz jalvz force-pushed the jalvz:index-sourcemap-path-only branch 2 times, most recently from 8754cd1 to 845fed4 Jan 15, 2019
@jalvz

This comment has been minimized.

Copy link
Collaborator Author

jalvz commented Jan 15, 2019

jenkins retest this, please

1 similar comment
@jalvz

This comment has been minimized.

Copy link
Collaborator Author

jalvz commented Jan 15, 2019

jenkins retest this, please

@jalvz jalvz force-pushed the jalvz:index-sourcemap-path-only branch from 025e290 to 287bbcb Jan 15, 2019
jalvz added 2 commits Dec 17, 2018
…s eg with many subdomains
@jalvz jalvz force-pushed the jalvz:index-sourcemap-path-only branch from 287bbcb to 79515a2 Jan 16, 2019
@jalvz jalvz merged commit 9abe228 into elastic:master Jan 16, 2019
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/jenkins/pr-merge This commit cannot be built
Details
apm-ci Build finished.
Details
CLA Commit author is a member of Elasticsearch
Details
@zube zube bot added [zube]: Done and removed [zube]: In Review labels Jan 16, 2019
jalvz added a commit to jalvz/apm-server that referenced this pull request Jan 16, 2019
* Index the url path only of the sourcemaps, so that sourcemapping works eg. with many subdomains

* Skip flaky test
jalvz added a commit to jalvz/apm-server that referenced this pull request Jan 16, 2019
* Index the url path only of the sourcemaps, so that sourcemapping works eg. with many subdomains

* Skip flaky test
jalvz added a commit that referenced this pull request Jan 16, 2019
* Index the url path only of the sourcemaps, so that sourcemapping works eg. with many subdomains

* Skip flaky test
jalvz added a commit that referenced this pull request Jan 16, 2019
* Index the url path only of the sourcemaps, so that sourcemapping works eg. with many subdomains

* Skip flaky test
jalvz added a commit to jalvz/apm-server that referenced this pull request Jan 16, 2019
* Index the url path only of the sourcemaps, so that sourcemapping works eg. with many subdomains

* Skip flaky test
jalvz added a commit to jalvz/apm-server that referenced this pull request Jan 16, 2019
* Index the url path only of the sourcemaps, so that sourcemapping works eg. with many subdomains

* Skip flaky test
jalvz added a commit to jalvz/apm-server that referenced this pull request Jan 16, 2019
* Index the url path only of the sourcemaps, so that sourcemapping works eg. with many subdomains

* Skip flaky test
jalvz added a commit to jalvz/apm-server that referenced this pull request Jan 16, 2019
* Index the url path only of the sourcemaps, so that sourcemapping works eg. with many subdomains

* Skip flaky test
jalvz added a commit to jalvz/apm-server that referenced this pull request Jan 16, 2019
* Index the url path only of the sourcemaps, so that sourcemapping works eg. with many subdomains

* Skip flaky test
jalvz added a commit to jalvz/apm-server that referenced this pull request Jan 16, 2019
* Index the url path only of the sourcemaps, so that sourcemapping works eg. with many subdomains

* Skip flaky test
jalvz added a commit to jalvz/apm-server that referenced this pull request Jan 16, 2019
* Index the url path only of the sourcemaps, so that sourcemapping works eg. with many subdomains

* Skip flaky test
jalvz added a commit that referenced this pull request Jan 16, 2019
* Index the url path only of the sourcemaps, so that sourcemapping works eg. with many subdomains

* Skip flaky test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.