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

parameters are not rendered by default #360

Closed
apotterri opened this issue Jul 30, 2024 · 2 comments · Fixed by #370
Closed

parameters are not rendered by default #360

apotterri opened this issue Jul 30, 2024 · 2 comments · Fixed by #370
Assignees
Labels
enhancement New feature or request navie-plan Plan the issue with Navie

Comments

@apotterri
Copy link
Contributor

apotterri commented Jul 30, 2024

Having the values of parameters in AppMap data isn't all that helpful. They're expensive to compute, and they take up a lot of space. Don't convert them to strings by default. Allow the user to turn on rendering (by setting APPMAP_DISPLAY_PARAMS=true).

@apotterri apotterri added the enhancement New feature or request label Jul 30, 2024
@apotterri apotterri self-assigned this Jul 30, 2024
@apotterri
Copy link
Contributor Author

apotterri commented Aug 8, 2024

I found that the errors in #319 that aren't related to pickling all go away when APPMAP_DISPLAY_PARAMS=true. Once this is implemented, the tests in https://github.com/pydicom/pydicom should be rerun.

@apotterri apotterri added the navie-plan Plan the issue with Navie label Aug 9, 2024
Copy link

github-actions bot commented Aug 9, 2024

Title: Disable Parameter Rendering by Default and Add Configuration Option for Enabling

Problem

Parameters are being rendered by default in AppMap data, which is resource-intensive and increases data size. Users should have the option to enable this feature by setting APPMAP_DISPLAY_PARAMS=true.

Analysis

Currently, AppMap renders the values of parameters automatically, and users need an option to disable this rendering by default. The configuration already exists to turn this feature on or off using the APPMAP_DISPLAY_PARAMS environment variable, but the default setting needs to be changed to not display parameters unless explicitly enabled.

The display_string and describe_value methods in the event.py file already reference the Env.current.display_params property to check if parameters need to be rendered. We will ensure that display_params is False by default unless APPMAP_DISPLAY_PARAMS=true is set.

Proposed Changes

  1. _appmap/env.py:

    • Modify the display_params property to return False by default unless APPMAP_DISPLAY_PARAMS is explicitly set to true.
  2. _appmap/event.py and _appmap/recorder.py:

    • Ensure that the display_params property in the environment configuration is correctly referenced where needed.

Detailed Changes

  1. _appmap/env.py:
@property
def display_params(self):
    """
    Return True if APPMAP_DISPLAY_PARAMS is set to 'true', otherwise False.
    """
    return self.get("APPMAP_DISPLAY_PARAMS", "false").lower() == "true"
  1. _appmap/event.py:
    • Ensure the display_string function does not render parameters unless Env.current.display_params is set to True.
# If we're asked to display parameters, make a best-effort attempt
# to get a string value for the parameter using repr(). If parameter
# display is disabled, or repr() has raised, just formulate a value
# from the class and id.
value = None
if Env.current.display_params:
    try:
        value = repr(val)
    except Exception:  # pylint: disable=broad-except
        pass
  1. _appmap/event.py:
    • Ensure the describe_value function does not process parameters deeply unless Env.current.display_params is set to True.
if Env.current.display_params:
    ret.update(_describe_schema(name, val, 0, max_depth))

Summary

By ensuring that rendering of parameter values is not done by default and can be enabled through the APPMAP_DISPLAY_PARAMS environment variable, we can save on computation and data storage. These changes will ensure that parameter values are rendered only if explicitly requested by the user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request navie-plan Plan the issue with Navie
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant