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

Add boolean config options #28

Merged
merged 2 commits into from
May 3, 2023
Merged

Add boolean config options #28

merged 2 commits into from
May 3, 2023

Conversation

tombruijn
Copy link
Member

@tombruijn tombruijn commented May 1, 2023

Add config options that are booleans and should be interpreted as such.

Part of #16
[skip changeset]

@backlog-helper
Copy link

backlog-helper bot commented May 1, 2023

✔️ All good!

New issue guide | Backlog management | Rules | Feedback

Copy link
Contributor

@unflxw unflxw left a comment

Choose a reason for hiding this comment

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

If active is false, should we not avoid attempting to start the agent (over at agent.py)? That is what the Node.js integration does (s/agent/extension)



DEFAULT_CONFIG = Options(
enable_host_metrics=True,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the default config include active=True as well? Otherwise users always have to specify it. Ruby derives it from whether push_api_key is non-empty.

Suggested change
enable_host_metrics=True,
active=True,
enable_host_metrics=True,

Copy link
Member Author

@tombruijn tombruijn May 2, 2023

Choose a reason for hiding this comment

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

In Ruby you also need to set it in the YAML file and Ruby code. The Push API key only sets it to true if its set through APPSIGNAL_PUSH_API_KEY. This way people can set it based on the env as well with something like: active=(os.environ.get("foo") == "true")

Copy link
Contributor

Choose a reason for hiding this comment

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

But people can do that anyway, right? The default will be overriden with whatever users decide to set... Perhaps the default should be false, then, like in the other integrations. Whatever makes sense. We'll need to amend the config in the test setup, though: https://github.com/appsignal/test-setups/blob/main/python/django4-celery/app/appsignal_config.py#L4

Copy link
Member Author

Choose a reason for hiding this comment

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

I figured I'd implement it the same way as the other integrations. We can always discuss behavior changes (please create an issue), but let's keep it like this for this PR.

tests/test_config.py Outdated Show resolved Hide resolved
Add config options that are booleans and should be interpreted as such.
@tombruijn tombruijn changed the base branch from remove-resource-attributes to main May 2, 2023 13:54
@tombruijn
Copy link
Member Author

If active is false, should we not avoid attempting to start the agent (over at agent.py)? That is what the Node.js integration does (s/agent/extension)

I've added another commit with this check for the active option.

src/appsignal/client.py Outdated Show resolved Hide resolved
@tombruijn tombruijn force-pushed the boolean-config-options branch 2 times, most recently from 6c9c504 to dafe175 Compare May 3, 2023 07:47
Ignore the start call if the config isn't set to active.

In the test I deliberately used an invalid config, so the agent will
exit automatically once started.
@tombruijn tombruijn merged commit 7e4b3fa into main May 3, 2023
1 check passed
@tombruijn tombruijn deleted the boolean-config-options branch May 15, 2023 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants