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.

Sign up for GitHub

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

Jump to bottom

Don't enable RUM agent if APM is run with contextPropagationOnly #118685

Merged
merged 3 commits into from
Nov 18, 2021
Merged

Don't enable RUM agent if APM is run with contextPropagationOnly #118685

merged 3 commits into from
Nov 18, 2021

Conversation

mshustov
Copy link
Contributor

Summary

Follow up for #112973

APM RUM agent shouldn't send data to APM server if APM nodejs is run with contextPropagationOnly or disableSend flags (https://github.com/elastic/apm/blob/master/specs/agents/transport.md#disable_send-configuration).

@mshustov mshustov added chore v8.0.0 release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v8.1.0 labels Nov 16, 2021
@mshustov mshustov marked this pull request as ready for review November 16, 2021 15:00
@mshustov mshustov requested a review from a team as a code owner November 16, 2021 15:00
@tylersmalley tylersmalley mentioned this pull request Nov 16, 2021
Closed
@trentm
Copy link
Member

trentm commented Nov 16, 2021

@mshustov Ultimately for #97934 would we ideally want the RUM agent to support the equivalent of contextPropagationOnly? If the RUM agent is disabled, then tracing will start at the Kibana backend, instead of possibly at the frontend if an ES query originated in the browser.

@mshustov
Copy link
Contributor Author

mshustov commented Nov 17, 2021 •
edited
Loading

@trentm in the long term, yes, we want. Right now, it's not a critical requirement.
The vast majority of the requests from the Kibana browser app are initiated asynchronously after a web page is loaded, and an appropriate transaction is finished. Even if we had more Kibana plugins creating APM transactions on the client-side, there is no way to propagate request context metadata due to the lack of async hooks API counterparts in the browser env. At best, we can find out from which URL a request was sent to the Kibana server. Which should be available in the http.server.response logs in http.request.referrer field anyway.
Let me know if I'm wrong, and we need to ask the RUM agent to prioritize this work.

mshustov
mshustov commented Nov 17, 2021
View reviewed changes
src/core/server/http_resources/get_apm_config.ts Outdated
import { getConfiguration } from '@kbn/apm-config-loader';

function shouldInstrumentClient(config?: AgentConfigOptions): boolean {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe this logic belongs to '@kbn/apm-config-loader'. I left it here as it was implemented here before.

Copy link
Member

Choose a reason for hiding this comment

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

First, I don't think you should block on me. I don't have a feel for Kibana code structure at all.

I think having all logic for APM config in @kbn/apm-config-loader might be less surprising.

@trentm
Copy link
Member

trentm commented Nov 17, 2021

Right now, it's not a critical requirement. ...

Understood. Makes sense.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/apm-config-loader 14 16 +2
Unknown metric groups

API count

id before after diff
@kbn/apm-config-loader 14 16 +2

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

vigneshshanmugam
vigneshshanmugam approved these changes Nov 17, 2021
View reviewed changes
Copy link
Member

@vigneshshanmugam vigneshshanmugam left a comment

Choose a reason for hiding this comment

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

LGTM

@mshustov mshustov merged commit 8a16b84 into elastic:main Nov 18, 2021
@mshustov mshustov deleted the rum-disabled-in-context-propagation-mode branch November 18, 2021 08:30
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 18, 2021
@kibanamachine
Don't enable RUM agent if APM is run with contextPropagationOnly (ela…
5a296aa 
…stic#118685)

* do not enable RUM agent when nodejs is run in contextPropagationOnly mode

* move shouldInstrumentClient to apm-config package
@kibanamachine kibanamachine mentioned this pull request Nov 18, 2021
Merged
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 8.0

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Nov 18, 2021
@kibanamachine
Don't enable RUM agent if APM is run with contextPropagationOnly (#11…
ca86b98 
…8685) (#118995)

* do not enable RUM agent when nodejs is run in contextPropagationOnly mode

* move shouldInstrumentClient to apm-config package

Co-authored-by: Mikhail Shustov <[email protected]>
TinLe pushed a commit to TinLe/kibana that referenced this pull request Nov 20, 2021
@TinLe
Don't enable RUM agent if APM is run with contextPropagationOnly (ela…
78737af 
…stic#118685)

* do not enable RUM agent when nodejs is run in contextPropagationOnly mode

* move shouldInstrumentClient to apm-config package
mshustov pushed a commit to mshustov/kibana that referenced this pull request Nov 24, 2021
Revert "Don't enable RUM agent if APM is run with contextPropagationO…
976e27a 
…nly (elastic#118685) (elastic#118995)"

This reverts commit ca86b98.
mshustov pushed a commit that referenced this pull request Nov 24, 2021
Revert "Include tracing information in the Kibana logs (#112973) (#11…
8dcf2b7 
…9567)

* Revert "Include tracing information in the Kibana logs (#112973) (#118604)"

This reverts commit 114c690.

* Revert "Don't enable RUM agent if APM is run with contextPropagationOnly (#118685) (#118995)"

This reverts commit ca86b98.
@mshustov mshustov removed the v8.0.0 label Nov 24, 2021
@mshustov mshustov added backport:skip This commit does not require backporting and removed auto-backport Deprecated - use backport:version if exact versions are needed labels Nov 24, 2021
dmlemeshko pushed a commit that referenced this pull request Nov 29, 2021
@dmlemeshko
Don't enable RUM agent if APM is run with contextPropagationOnly (#11…
75b04cd 
…8685)

* do not enable RUM agent when nodejs is run in contextPropagationOnly mode

* move shouldInstrumentClient to apm-config package
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trentm trentm trentm left review comments

@Bamieh Bamieh Bamieh approved these changes

@vigneshshanmugam vigneshshanmugam vigneshshanmugam approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting chore release_note:skip Skip the PR/issue when compiling release notes v8.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mshustov @trentm @kibana-ci @kibanamachine @vigneshshanmugam @Bamieh