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.
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
feat: fetch sourcemaps from Elasticsearch #9722
feat: fetch sourcemaps from Elasticsearch #9722
Changes from 82 commits
8bcf7b4
3d61fc1
d18afa0
6b13bb3
27883df
4d8c011
f04473e
7a64c06
c96587d
855cb02
cd01a10
9890493
9518384
761802b
ac529cd
a9957ea
260d3d5
995e254
cf86bb3
813cdfb
6f0fb18
094dbfc
3f0c2be
cc35612
a3e65cb
b1a2bf2
0f4b3fa
a5ea7bf
3041003
9486691
b0b8646
0942d3d
3d72922
77ed8f1
5e80d77
61fed2d
2131a33
4d66334
16839e8
1031ac2
413385a
4532dfb
cff8dd0
7946c09
39510c4
020f869
870b635
06a59b1
b881da3
df99000
ffa8941
c5508d9
b342665
695c495
b3c5792
b48a882
7aded27
ba117b4
d4fccb2
54ab5b9
1aa9654
529b129
778ec6d
69e2859
faece2a
8c03743
878b025
558a473
7df5419
e186512
c194073
b56a1ea
7757b55
0b38f0d
a890a2f
174e87e
d12ebdf
90bc423
88f78a0
873b4c6
c919996
b674d5d
4bc95f7
e13870b
3066ce0
42bc926
f258815
0c34f3c
444c55b
b7389a6
4610931
ed99eb5
29d329d
cd6ae96
e2b24a0
0c3eef4
1d4d261
240da96
26ecdfe
cf94883
5c92302
c8ca86d
c593dfc
c87ce27
cc5a01a
f353cd1
04dc1fb
88c8011
824c68f
9efecb9
d466045
ed6c474
ffaef7f
c4e9407
6d1472c
ab147c5
dccaec4
067202b
5ff04ac
e64ddb3
a65af2a
0db75d9
711ed63
9c19470
55f95c9
868eb55
be3ea36
54be00c
17b485b
557e4a3
af66eda
6eb7746
2ac90e0
a44b82a
ee25e37
11c097b
3530b49
14d230a
d8924cc
3ef10e5
bbebe6a
c24db8d
08ae602
5b0dfb6
2e814f4
53c7d97
8028c46
8c61b76
4de127a
7b75446
a182dd2
1d74a28
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't aware that the actual sourcemap index in ES changes, we had discussed that the index already exists. With this change, existing standalone APM Server that rely on fetching sourcemaps from ES might be broken on upgrade, as the configured credentials need to have privileges to query this index.
Can you clarify why the index itself changed and if this is really necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct. Both the index and the way sourcemaps are stored in ES changed. This happened in the kibana PR (https://github.com/elastic/kibana/pull/147208/files#diff-a968c157df4cd16bb68294e0741431359aba6574abbed2020b7ca10e4f592159R28), it is necessary and we are just defaulting to what they use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree that as long as the Kibana side changes this, we have to adapt, but I'd like to question whether the changes in the Kibana UI code are necessary. @kruskall and I discussed that he will follow up with the UI folks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was of the impression that
apm-*-sourcemap*
was no longer used, and the ability to configure the index was removed to simplify things:elastic/kibana@4050578
My concern was that if the end user could configure the source map index, the internal kibana user might not have permissions to query it. With a hardcoded index (
.apm-source-map
) I was sure this was not the case.I can easily change this back to
apm-{version}-sourcemap
. It won't be user configurable but it'll match the old pattern.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw. I suppose we still need to create the index for recent versions of the stack. Or does APM Server still create the index at startup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, and this is a valid concern.@kruskall could you check how the sourcemap index is currently setup (before your changes) when the server runs in standalone mode and Kibana is not enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this solve the problem on your end? elastic/kibana#149403
Btw I changed the index to
apm-agent-sourcemap
so it still matchesapm-*-sourcemap*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed that we would keep the currently implemented logic and move forward with
.apm-source-map
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd completely remove
SourceMapping.Metadata
. Afaics this is not used anywhere anymore and was only used with the FleetFetcher, so it should never reach this code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not used in the APM Server but I think kibana (?) might be injecting this option.
For example, in
TestRUMErrorSourcemapping
this code path is triggered and we're logging a warning.I agree we can remove it here 👍