add input and output formatter to decorator for custom formats

[dinmukhamedm]

Creating a draft to try and run tests here. For some reason they don't run for me on local

---

Important

Add input and output formatters to observe decorator for custom input/output formatting, with refactoring and new tests.

• Decorators:
  • Add input_formatter and output_formatter to observe_base and async_observe_base for custom input/output formatting.
  • Refactor span setup, input/output processing, and cleanup into _setup_span, _process_input, _process_output, and _cleanup_span.
• Tests:
  • Add tests for input_formatter and output_formatter in test_observe.py.
  • Test cases cover normal operation, exceptions, and async functions.

^{This description was created by for 8c5166d. You can customize this summary. It will automatically update as commits are pushed.}

[greptile-apps]

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed everything up to 25b869d in 3 minutes and 45 seconds. Click for details.

• Reviewed 508 lines of code in 3 files
• Skipped 0 files when reviewing.
• Skipped posting 5 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. src/lmnr/opentelemetry_lib/decorators/__init__.py:202

• Draft comment:
  In async_observe_base, the 'with get_tracer() as tracer:' block is redundant since _setup_span already obtains a tracer instance. Consider removing the extra context manager.
• Reason this comment was not posted:
  Comment was on unchanged code.

2. src/lmnr/opentelemetry_lib/decorators/__init__.py:212

• Draft comment:
  Similarly, in the async decorator exception block, ensure that the context token is detached on exceptions to avoid leaks.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the author to ensure a specific behavior, which violates the rule against asking the author to ensure behavior is intended or tested. It doesn't provide a specific suggestion or ask for a specific test to be written.

3. src/lmnr/sdk/decorators.py:105

• Draft comment:
  Tags provided are retained as a list, but tests expect a tuple. Consider converting the tags to a tuple (e.g. association_properties['tags'] = tuple(tags)) to meet test expectations.
• Reason this comment was not posted:
  Comment was on unchanged code.

4. src/lmnr/sdk/decorators.py:61

• Draft comment:
  Typo/Consistency Issue: The docstring for input_formatter mentions "Ignored if ignore_input is True." but then says "Does not respect ignore_inputs argument." Consider using consistent naming (either ignore_input or ignore_inputs) to avoid confusion.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment appears to misunderstand that ignore_input and ignore_inputs are two distinct parameters with different purposes. The docstring is actually correct - the formatter is ignored if ignore_input is True (all input ignored), and separately, the formatter doesn't respect the ignore_inputs list (specific inputs to ignore). These are not inconsistent statements, they're describing different behaviors with different parameters. Could there still be value in making the distinction between these parameters clearer in the docstring to prevent other developers from having the same confusion? While clarity is good, the docstring is already technically accurate, and the parameters' purposes are well-documented in their own sections. This comment is based on a misunderstanding rather than identifying a real issue. The comment should be deleted because it incorrectly identifies an inconsistency where there isn't one - the docstring is accurate in describing how the formatter interacts with two different parameters that serve different purposes.

5. src/lmnr/sdk/decorators.py:66

• Draft comment:
  Typo/Consistency Issue: The docstring for output_formatter mentions "Ignored if ignore_output is True." but then says "Does not respect ignore_inputs argument." Likely this should be corrected to refer to ignore_output (or a consistent parameter name) for clarity.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

Workflow ID: wflow_RShr6pPI3CZWzpE9

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Both _process_input and _process_output catch only TypeError. Consider broadening exception handling or logging unexpected exceptions from custom formatter functions to aid debugging.

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 68feecd in 1 minute and 23 seconds. Click for details.

• Reviewed 85 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 5 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. src/lmnr/opentelemetry_lib/decorators/__init__.py:97

• Draft comment:
  Catching Exception here (and below) instead of a more specific exception may mask unintended errors. If this behavior is intended, please add a comment explaining why all Exceptions are ignored.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 50% <= threshold 50% The comment suggests that catching a general Exception might mask unintended errors, which is a valid concern in coding practices. However, it asks the author to confirm their intention and to add a comment explaining the behavior, which violates the rule against asking for confirmation or explanation. The suggestion to use a more specific exception is valid, but the request for an explanation is not allowed.

2. src/lmnr/opentelemetry_lib/decorators/__init__.py:124

• Draft comment:
  As with _process_input, catching a broad Exception here could hide errors in the output formatter. Consider documenting this design or catching only expected exceptions.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is telemetry code that should never break the main flow. The broad exception catch with logging is actually better than the original TypeError catch, as it provides visibility into failures while maintaining robustness. The comment seems to misunderstand the purpose of this error handling - we want to catch everything here because span attribute setting should never break the application. I might be underestimating the value of catching specific exceptions. There could be particular error cases that deserve special handling. In observability code, failing silently while logging is the right pattern. Catching specific exceptions would add complexity without benefit since we handle all errors the same way. The comment should be deleted. The broad exception catch is the correct pattern here for telemetry code, and the added logging improves visibility.

3. tests/test_observe.py:302

• Draft comment:
  Good test coverage for handling formatter exceptions. The test ensures that when the input formatter raises an exception, the span lacks the corresponding attribute.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

4. tests/test_observe.py:390

• Draft comment:
  Similarly, the test for output formatter exception confirms that no output attribute is set on failure. Nice coverage.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

5. src/lmnr/opentelemetry_lib/decorators/__init__.py:20

• Draft comment:
  There's an unexpected ) on line 20. This appears to be a typographical error—please remove or correct it if it was intended to be something else.
• Reason this comment was not posted:
  Comment was on unchanged code.

Workflow ID: wflow_qDQy3Ot9CtX7g52G

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[skull8888888]

let's also fix, "can't serialize input" errors

[dinmukhamedm]

@skull8888888 let's say the inputs are:

def foo(a: int, b: str, c: MyNonSerializableClass) -> int:
    pass

There are 3 options on what we could do now:

1. [Current] Ignore, i.e. not set lmnr.span.input, possibly logging the warning msg
2. Ignore just c, setting span lmnr.span.input to {"a": 42, "b": "hey"}, possibly logging a warning msg
3. Ignore just c, but mark the skip, setting span lmnr.span.input to {"a": 42, "b": "hey", "c": "<instance of MyNonSerializableClass>"}, possibly logging a warning msg

I'm leaning towards option 3, but option 2 isn't bad either. What do you think? Also shall we log the warning?

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 8c5166d in 1 minute and 34 seconds. Click for details.

• Reviewed 30 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 2 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. tests/test_observe.py:667

• Draft comment:
  Substring matching on the object's repr ("NonSerializable object at 0x") may be brittle. Consider using a regex or comparing with repr(instance) to account for variation in memory addresses.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The test is verifying basic functionality - that non-serializable objects get converted to strings in a predictable way. The current substring check is simple and adequate for this purpose. Making it more robust with regex would add complexity without significant benefit. The memory address variation isn't important to the test's purpose. The comment does identify a real potential brittleness in the test. If the object's string representation format changed, the test could break. However, the test's purpose is just to verify the basic string conversion behavior, not to be a comprehensive test of object representation. The current approach is simple and clear. The comment suggests an over-optimization that would add complexity without meaningful benefit. The current test approach is appropriate for its purpose.

2. tests/test_observe.py:668

• Draft comment:
  Ensure the fallback formatting for non-serializable outputs is consistent. Using json.loads() on the output attribute and checking for a substring ties the test to the default repr behavior.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment raises a valid point about test brittleness - relying on the exact string format of Python's default repr() could make the test fragile. However, this is a test of the observe() decorator's behavior with non-serializable objects, and checking for this string pattern is a reasonable way to verify the fallback behavior. The test needs some way to verify the behavior. The comment starts with "Ensure that..." which violates our rules about not asking authors to verify things. Additionally, while the point about test brittleness is valid, the comment doesn't provide a clear alternative approach. While test brittleness is worth considering, some degree of implementation dependence is unavoidable when testing fallback behavior. The current approach is pragmatic and clear. Delete the comment. While it raises a valid concern about test brittleness, it violates our rules by asking the author to verify/ensure something, and doesn't provide actionable alternatives.

Workflow ID: wflow_DDhk0h4RZcxzRfiD

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}