-
Notifications
You must be signed in to change notification settings - Fork 17
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 a kitchen sink #226
Add a kitchen sink #226
Conversation
## Plugins | ||
### Core Agent | ||
|
||
By default, the IOpipe agent comes pre-loaded with all the bundled plugins in `iopipe.contrib.*`. If you prefer to run the agent without plugins or configure which plugins are used, you can use `IOpipeCore`: |
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.
Shouldn't this be choosy about what it includes, if it's following the model of the node agent?
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.
Addendum: should we mention the plugins it includes?
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.
The docs say the preloaded plugins are the ones in iopipe.contrib.*
, but I can be more explicit in the docs. We don't need to exclude the profiler plugin from the Python agent as it's disabled by default and adds no overhead when preloaded.
README.md
Outdated
pass | ||
``` | ||
|
||
Although any plugins that add overhead are disabled by default when loaded, so we recommend using the default agent. |
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.
Phrasing is weird here
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.
Weird how?
return IOpipe('test-suite', 'https://metrics-api.iopipe.com', True) | ||
|
||
|
||
@pytest.fixture | ||
def default_iopipe_override(): | ||
return IOpipe('test-suite', 'https://metrics-api.iopipe.com', True, plugins=[TracePlugin(auto_measure=False)]) |
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.
So when someone wants to leave a plugin in kitchen sink out for whatever reason, they use core? (I think this is true and what we would recommend, but want to be sure)
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.
Yes, this is how the Node agent works, too. Although, unlike the Node agent, you can override the plugins if you want to change how a preloaded plugin is configured.
README.md
Outdated
```python | ||
from iopipe import IOpipeCore | ||
|
||
iopipe = IOpipeCore() |
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.
This example would be more helpful if it has a plugins
setting and a comment clarifying that it means the agent will be set up with only those plugins
@@ -34,7 +34,7 @@ def __init__(self, enabled=False): | |||
|
|||
@property | |||
def enabled(self): | |||
return self._enabled is True or strtobool(os.getenv('IOPIPE_PROFILER_ENABLED', 'false')) | |||
return self._enabled is True or bool(strtobool(os.getenv('IOPIPE_PROFILER_ENABLED', 'false'))) |
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 have to cast strtobool as a bool?
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.
Found an edge case where if you do strtobool('0')
it returns an int
and not a bool
.
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.
Interesting, thanks!
README.md
Outdated
```python | ||
from iopipe import IOpipeCore | ||
|
||
iopipe = IOpipeCore(plugins=[]) |
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.
The prior suggestion was looking for more like:
from iopipe import IOpipeCore
from iopipe.contrib.trace import TracePlugin
# Load IOpipe agent with only the tracing plugin
iopipe = IOpipeCore(plugins=[TracePlugin()])
acceptance/serverless/handler.py
Outdated
|
||
profiler_plugin = ProfilerPlugin(enabled=True) | ||
iopipe_with_profiling = IOpipe(debug=True, plugins=[profiler_plugin]) | ||
iopipe_with_profiling = IOpipeCore(debug=True, plugins=[profiler_plugin]) | ||
|
||
iopipe_with_sync_http = IOpipe(sync_http=True) |
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.
Shouldn't we have a kitchen sink running with concurrent requests?
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.
Yes, all the other agents run with concurrent requests.
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.
And success
is kitchen sink.
configured_plugins = kwargs.pop('plugins', None) | ||
plugins = [EventInfoPlugin(), LoggerPlugin(), ProfilerPlugin(), TracePlugin()] | ||
if configured_plugins is not None and isinstance(plugins, list): | ||
plugins = plugins + configured_plugins |
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.
Huh, this de-dupes plugins?
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.
Yes, see load_plugins
in IOpipeCore
.
Closes iopipe#224 Signed-off-by: Michael Lavers <[email protected]>
Signed-off-by: Michael Lavers <[email protected]>
Signed-off-by: Michael Lavers <[email protected]>
Signed-off-by: Michael Lavers <[email protected]>
Signed-off-by: Michael Lavers <[email protected]>
Signed-off-by: Michael Lavers <[email protected]>
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.
Minor nit about comments/understanding why the plugins list is reversed/how that contributes to the de-duping of plugins when users provide plugins. Otherwise, LGTM 👍
iopipe/agent.py
Outdated
for plugin in reversed(plugins): | ||
if not is_plugin(plugin) or plugin.name in plugins_seen: | ||
continue | ||
# Build the plugins list in reverse to restore original order |
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 looks like this comment belongs above 186, and it'd be helpful to clarify that you're using the "reverse order, so that user plugins override kitchen sink* plugins" *use a name that makes sense
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.
No, the comment is in the correct location, as that's where the list is being built. Added an additional comment above the loop to explain why we're iterating over the plugins in reverse order.
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.
Sounds good
Signed-off-by: Michael Lavers <[email protected]>
Closes #224
Signed-off-by: Michael Lavers [email protected]