feat: Add APPMAP_DISPLAY_LABELED_PARAMS to capture params for labeled functions by kgilpin · Pull Request #384 · getappmap/appmap-python

kgilpin · 2026-03-31T23:48:47Z

Adds a new env var APPMAP_DISPLAY_LABELED_PARAMS (default: true) that enables str and repr()-based parameter and return value capture for functions that have labels, even when APPMAP_DISPLAY_PARAMS is off. This provides useful parameter visibility for semantically important functions (HTTP, crypto, serialization, etc.) without the performance cost of capturing all parameters globally.

Copilot

Pull request overview

Adds a new configuration knob to selectively capture parameter/return-value details (via repr() and schema) for labeled functions even when global parameter capture is disabled, improving visibility for semantically important calls without enabling full parameter capture everywhere.

Changes:

Introduces APPMAP_DISPLAY_LABELED_PARAMS (default true) and threads it through initialization and Env.
Propagates “has labels” metadata through instrumentation so event value rendering can decide whether to display details.
Adds tests and test fixtures validating labeled vs unlabeled behavior under different env settings.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`appmap/__init__.py`	Initializes `_APPMAP_DISPLAY_LABELED_PARAMS` from `APPMAP_DISPLAY_LABELED_PARAMS` (default `true`) when AppMap is enabled.
`_appmap/env.py`	Adds `display_labeled_params` to `Env` for runtime checks.
`_appmap/event.py`	Adds `_should_display(has_labels)` gating and plumbs `has_labels` into parameter/return value description logic.
`_appmap/instrument.py`	Detects whether the function has labels and forwards `has_labels` into param/return event creation.
`_appmap/test/conftest.py`	Ensures tests initialize `_APPMAP_DISPLAY_LABELED_PARAMS` for deterministic behavior.
`_appmap/test/test_events.py`	Adds coverage for labeled/unlabeled param/return display behavior under env combinations.
`_appmap/test/data/example_class.py`	Adds a labeled method that takes a parameter for exercising the new behavior in tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

appmap/__init__.py

_appmap/env.py

_appmap/test/test_events.py

dividedmind

I like this change, however in my opinion there's a better UX than adding another envar: APPMAP_DISPLAY_PARAMS could be true to force all parameters being captured, false to none, and unset (or, maybe, set to auto or labeled? in any case, the default) to only capture on the labeled functions. WDYT? Having two variables can be confusing — it's unclear what their interaction is.

dividedmind · 2026-04-04T12:49:40Z

I like this change, however in my opinion there's a better UX than adding another envar: APPMAP_DISPLAY_PARAMS could be true to force all parameters being captured, false to none, and unset (or, maybe, set to auto or labeled? in any case, the default) to only capture on the labeled functions. WDYT? Having two variables can be confusing — it's unclear what their interaction is.

@kgilpin , I went ahead and implemented this change, can you take a look if it looks okay? Note when merging the commits should be squashed and a new unified commit message written.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-04T12:52:33Z

appmap/__init__.py

+        _display_params = os.environ.get("APPMAP_DISPLAY_PARAMS", "labeled")
        os.environ.setdefault("_APPMAP_DISPLAY_PARAMS", _display_params)


PR title/description mention a new env var APPMAP_DISPLAY_LABELED_PARAMS, but the implementation here instead changes the default and accepted values of APPMAP_DISPLAY_PARAMS (e.g. defaulting to "labeled"). There are no references to APPMAP_DISPLAY_LABELED_PARAMS in the repo, so either the new env var needs to be implemented as described, or the PR metadata/docs should be updated to match the actual behavior and configuration surface.

This shall be resolved in the final commit message. I'm reluctant to change the description and title of this collaborative PR to not distort the history.

_appmap/env.py

Copilot · 2026-04-04T12:52:34Z

_appmap/env.py

+        display_params = self._env.get("_APPMAP_DISPLAY_PARAMS", "labeled").lower()
+        if display_params == "true":
+            self._display_params = True
+            self._display_labeled_params = True
+        elif display_params == "false":
+            self._display_params = False
+            self._display_labeled_params = False
+        else: # "labeled" or "auto" or anything else defaults to labeled
+            self._display_params = False
+            self._display_labeled_params = True


The parsing logic treats any value other than "true"/"false" as "labeled" (including typos). That makes it easy to accidentally enable labeled parameter capture when the intent was to disable it. Consider validating against an explicit allowlist (e.g., true/false/labeled), and for invalid values log a warning and fall back to the safest behavior (typically fully disabled).

In principle you're right, but I'm not very concerned about it. The other variables aren't defensive like this and I don't think we should make this one any different.

_appmap/test/test_events.py

kgilpin · 2026-04-04T14:21:04Z

I like this change, however in my opinion there's a better UX than adding another envar: APPMAP_DISPLAY_PARAMS could be true to force all parameters being captured, false to none, and unset (or, maybe, set to auto or labeled? in any case, the default) to only capture on the labeled functions. WDYT? Having two variables can be confusing — it's unclear what their interaction is.

I would have been fine with it either way so I'm ok with this. It makes it tricky to add another case - but we don't have plans for another case anyway.

kgilpin · 2026-04-04T14:22:14Z

I like this change, however in my opinion there's a better UX than adding another envar: APPMAP_DISPLAY_PARAMS could be true to force all parameters being captured, false to none, and unset (or, maybe, set to auto or labeled? in any case, the default) to only capture on the labeled functions. WDYT? Having two variables can be confusing — it's unclear what their interaction is.

@kgilpin , I went ahead and implemented this change, can you take a look if it looks okay? Note when merging the commits should be squashed and a new unified commit message written.

LGTM. You're actually the reviewer so I can't Approve the PR (I opened it).

🛳️

Thanks for contributing to this PR!

Introduces a new mode to APPMAP_DISPLAY_PARAMS, which now can be: - 'true': capture argument values for all functions, - 'false': disable all argument value capture, - 'labeled' (new, default): capture only for labeled functions. The labeled default provides useful argument visibility for semantically important functions (such as HTTP, crypto, etc.) without the performance cost of global capture. The capture policy is evaluated once per function at instrumentation time and cached on the `_InstrumentedFn` tuple, eliminating per-call environment lookups from the hot path. Co-Authored-By: kgilpin <kgilpin@gmail.com>

kgilpin requested review from Copilot and dividedmind March 31, 2026 23:48

kgilpin self-assigned this Mar 31, 2026

Copilot started reviewing on behalf of kgilpin March 31, 2026 23:49 View session

Copilot AI reviewed Mar 31, 2026

View reviewed changes

appmap/__init__.py Outdated Show resolved Hide resolved

_appmap/env.py Outdated Show resolved Hide resolved

_appmap/test/test_events.py Outdated Show resolved Hide resolved

dividedmind requested changes Apr 1, 2026

View reviewed changes

dividedmind force-pushed the feat/display-labeled-params branch from 6471e4d to 1e60a85 Compare April 4, 2026 12:48

dividedmind requested a review from Copilot April 4, 2026 12:48

Copilot started reviewing on behalf of dividedmind April 4, 2026 12:48 View session

Copilot AI reviewed Apr 4, 2026

View reviewed changes

dividedmind force-pushed the feat/display-labeled-params branch from 1e60a85 to 404a74d Compare April 4, 2026 13:31

dividedmind force-pushed the feat/display-labeled-params branch from 404a74d to a32893d Compare April 4, 2026 16:07

dividedmind and others added 2 commits April 4, 2026 18:18

chore: move cyclic-imports pylint exclusion from tox.ini to pylintrc

1d81723

dividedmind force-pushed the feat/display-labeled-params branch from a32893d to b324b48 Compare April 4, 2026 16:25

dividedmind approved these changes Apr 4, 2026

View reviewed changes

dividedmind merged commit 453b697 into master Apr 4, 2026
7 checks passed

dividedmind deleted the feat/display-labeled-params branch April 4, 2026 16:31

		_display_params = os.environ.get("APPMAP_DISPLAY_PARAMS", "labeled")
		os.environ.setdefault("_APPMAP_DISPLAY_PARAMS", _display_params)

Conversation

kgilpin commented Mar 31, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dividedmind left a comment

Choose a reason for hiding this comment

Uh oh!

dividedmind commented Apr 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

Uh oh!

dividedmind Apr 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

Uh oh!

dividedmind Apr 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kgilpin commented Apr 4, 2026

Uh oh!

kgilpin commented Apr 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants