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

Service specific source maps #5410

Merged

Conversation

stuartnelson3
Copy link
Contributor

@stuartnelson3 stuartnelson3 commented Jun 3, 2021

Motivation/summary

support configuring source maps from kibana and fetching them from fleet server.

closes #5002

Checklist

How to test these changes

Run elasticsearch + and elastic-agent in fleet mode.

Start kibana with the code from elastic/kibana#95393 has been completed, and apm-server with the fleet-server sourcemap code.

  • Upload a sourcemap via kibana
  • Confirm an updated config with the new source_maps block has been pushed to apm-server
  • Confirm ingesting rum traces for the uploaded sourcemap get mapped appropriately

Related issues

depends on elastic/kibana#95393

@apmmachine
Copy link
Contributor

apmmachine commented Jun 3, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #5410 updated

  • Start Time: 2021-06-23T09:36:35.727+0000

  • Duration: 53 min 25 sec

  • Commit: d098115

Test stats 🧪

Test Results
Failed 0
Passed 6161
Skipped 119
Total 6280

Trends 🧪

Image of Build Times

Image of Tests

Re-use the RoundTripper configuration code for the
es client to configure the fleet http client the
same way.

Internally, the es client is instrumented, so do
that as well.
Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach looks good. The biggest unknown for me is where we get TLS config from for communicating with fleet-server.

beater/config/rum.go Outdated Show resolved Hide resolved
sourcemap/store.go Outdated Show resolved Hide resolved
sourcemap/fleet_store.go Outdated Show resolved Hide resolved
beater/config/sourcemapping.go Outdated Show resolved Hide resolved
beater/beater.go Outdated Show resolved Hide resolved
this was a temporary fix
if a series of requests come in at once for a key
that isn't in the cache, coordinate a single
request to the backend to retrieve it and have the
others wait until the request is complete.
Copy link
Contributor

@simitt simitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a first high-level review.

_meta/beat.yml Outdated Show resolved Hide resolved
beater/beater.go Outdated Show resolved Hide resolved
sourcemap/store.go Outdated Show resolved Hide resolved
sourcemap/store.go Show resolved Hide resolved
Limit the number of open requests that are
waiting for a response from the sourcemap backend
we won't be re-using elasticsearch config for
fleet-server configuration.
@stuartnelson3 stuartnelson3 marked this pull request as ready for review June 7, 2021 15:37
@mergify
Copy link
Contributor

mergify bot commented Jun 8, 2021

This pull request is now in conflicts. Could you fix it @stuartnelson3? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b service-specific-source-maps upstream/service-specific-source-maps
git merge upstream/master
git push upstream service-specific-source-maps

@stuartnelson3 stuartnelson3 marked this pull request as draft June 8, 2021 11:57
axw and others added 2 commits June 22, 2021 10:37
Remove the TestDataStreams tests, which test the
server's internal/private/undocumneted configuration
for enabling data streams. At least for now, this is
only intended to be used with Fleet, so we should
instead just rely on the Fleet system test.

There was a bug in the Fleet test where apm-server
would create "traces-apm-default" as a plain old
index instead of a data stream in certain conditions:
if the apm integration had previously been installed,
then the traces-apm template would be deleted by
CleanupElasticsearch. Because the package was already
installed, creating the policy would not automatically
reinstall it and add the missing template.

We fix the Fleet issue by uninstalling the package
during Fleet cleanup, after unenrolling agents.
@stuartnelson3
Copy link
Contributor Author

@axw let me know if you spot anything else when you get the chance

Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a few more minor comments, otherwise LGTM!

beater/beater_test.go Show resolved Hide resolved
sourcemap/fleet_store.go Outdated Show resolved Hide resolved
sourcemap/fleet_store_test.go Outdated Show resolved Hide resolved
@stuartnelson3
Copy link
Contributor Author

jenkins run the tests

@stuartnelson3 stuartnelson3 merged commit 9453fe2 into elastic:master Jun 23, 2021
@stuartnelson3 stuartnelson3 deleted the service-specific-source-maps branch June 23, 2021 11:27
mergify bot pushed a commit that referenced this pull request Jun 23, 2021
Serve service-specific sourcemaps from fleet-server

(cherry picked from commit 9453fe2)

# Conflicts:
#	apmpackage/apm/manifest.yml
#	changelogs/head.asciidoc
stuartnelson3 added a commit that referenced this pull request Jun 23, 2021
* Service specific source maps (#5410)

Serve service-specific sourcemaps from fleet-server

(cherry picked from commit 9453fe2)

# Conflicts:
#	apmpackage/apm/manifest.yml
#	changelogs/head.asciidoc

* Update manifest.yml

* Delete head.asciidoc

Co-authored-by: stuart nelson <[email protected]>
@axw axw self-assigned this Jul 12, 2021
mergify bot pushed a commit that referenced this pull request Jul 12, 2021
Serve service-specific sourcemaps from fleet-server

(cherry picked from commit 9453fe2)

# Conflicts:
#	apmpackage/apm/manifest.yml
#	beater/beater.go
#	beater/beater_test.go
#	changelogs/head.asciidoc
#	sourcemap/fleet_store.go
#	sourcemap/fleet_store_test.go
#	sourcemap/store_test.go
#	tests/system/test_integration_sourcemap.py
@axw
Copy link
Member

axw commented Jul 16, 2021

I tested this with the current Kibana API which requires sending the source map as a string, rather than as a file.

It works, but there's a small issue. I essentially manually ran this system test, but using the Kibana API for uploading the source map:

func TestRUMErrorSourcemapping(t *testing.T) {
systemtest.CleanupElasticsearch(t)
srv := apmservertest.NewUnstartedServer(t)
srv.Config.RUM = &apmservertest.RUMConfig{Enabled: true}
err := srv.Start()
require.NoError(t, err)
uploadSourcemap(t, srv, "../testdata/sourcemap/bundle.js.map",
"http://localhost:8000/test/e2e/../e2e/general-usecase/bundle.js.map",
"apm-agent-js",
"1.0.1",
)
systemtest.Elasticsearch.ExpectDocs(t, "apm-*-sourcemap", nil)
systemtest.SendRUMEventsPayload(t, srv, "../testdata/intake-v2/errors_rum.ndjson")
result := systemtest.Elasticsearch.ExpectDocs(t, "apm-*-error", nil)
systemtest.ApproveEvents(
t, t.Name(), result.Hits.Hits,
// RUM timestamps are set by the server based on the time the payload is received.
"@timestamp", "timestamp.us",
)
}

The issue is that the "bundle_filepath" is apparently not being cleaned. The path with "e2e/../e2e" in it does not match, but when I manually clean it (i.e. change that substring to "e2e"), then it does match.

The existing APM Server endpoint cleans the URL path before storing it in Elasticsearch:

smap.BundleFilepath = utility.CleanUrlPath(req.FormValue("bundle_filepath"))

Kibana should probably do the same. Alternatively we could clean it after receiving it in the APM Server, but seems better to store it cleaned in the first place. I'll open a Kibana issue.

@axw
Copy link
Member

axw commented Jul 19, 2021

Calling this one working as expected, there's elastic/kibana#105911 open to sort out the issue noted above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce configuration for specifying service-specific source maps
6 participants