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

introduce enabled/recording settings #790

Merged
merged 10 commits into from
May 1, 2020

Conversation

beniwohli
Copy link
Contributor

What does this pull request do?

Introduce recording/enabled as per elastic/apm#92 (comment)

Copy link
Contributor

@basepi basepi left a comment

Choose a reason for hiding this comment

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

I may be missing something, but I'm not sure I can see code paths for stopping the agent when enabled is set to False via remote config. It appears that all enabled code paths are only hit on startup, around starting threads and instrumenting code. We need to be able to disable the agent in mid-run as well via remote config, right?

@beniwohli
Copy link
Contributor Author

@basepi the "spec" (a.k.a. random comment amidst lengthy discussion) says

Introduce enabled config: true by default, setting to false will completely disable the agent, including instrumentation and remote config polling. This can only be set locally to the agent (e.g. in code, config file, environment variable), and is interpreted once at agent initialisation time; once disabled, disabled for the process's lifetime.

That's the only place I'm aware of that defines behavior of these new settings. Is there any other place I missed?

@basepi
Copy link
Contributor

basepi commented Apr 20, 2020

Oh! I missed that. I thought enabled was a remote config option as well. We always talk about enabled/recording together in the remote config issue: elastic/apm#213

That certainly makes things easier, thanks for the explanation.

Copy link
Contributor

@basepi basepi left a comment

Choose a reason for hiding this comment

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

Great work on this! Definitely simpler and cleaner without having to deal with enabled from remote config, that was the piece that I was planning to spend the most time one. Probably good you got to this one since you had internalized the requirements better than I had. 👍

@apmmachine
Copy link
Contributor

apmmachine commented Apr 28, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview stats

Expand to view the summary

Build stats

Test stats 🧪

Test Results
Failed 0
Passed 9193
Skipped 6667
Total 15860

@basepi
Copy link
Contributor

basepi commented Apr 30, 2020

@beniwohli if this is ready let's get it merged on Monday and cut a new release early next week.

@beniwohli beniwohli merged commit 5cdb443 into elastic:master May 1, 2020
@beniwohli beniwohli deleted the enabled-recording branch May 1, 2020 10:56
romulorosa pushed a commit to romulorosa/apm-agent-python that referenced this pull request Oct 15, 2020
* introduce enabled/recording settings

see elastic/apm#92 (comment)

Co-authored-by: Colton Myers <colton.myers@gmail.com>
romulorosa pushed a commit to romulorosa/apm-agent-python that referenced this pull request Oct 15, 2020
* introduce enabled/recording settings

see elastic/apm#92 (comment)

Co-authored-by: Colton Myers <colton.myers@gmail.com>
beniwohli added a commit to beniwohli/apm-agent-python that referenced this pull request Sep 14, 2021
* introduce enabled/recording settings

see elastic/apm#92 (comment)

Co-authored-by: Colton Myers <colton.myers@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants