Added MetricsLogger.add_stack_trace()

[benkehoe]

Implements #38

Description of changes: Added the MetricsLogger.add_stack_trace() method, which takes a key, an optional value, and an optional exc_info tuple (if not given, it uses sys.exc_info()). It adds a property using that key, with the following value:

{
  "property_key": {
    "value": "optional_value", // only if present
    "error_type": "module.SomeError", // module omitted for builtins
    "error_str": "result of str(e)",
    "stack_trace": [
      // traceback.format_tb()
    ]
}

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[benkehoe]

Note flake8 is failing because of a warning for the import in aws_embedded_metrics/__init__.py, which I also fixed in #42

[benkehoe]

I chose to make the values null when there's no current exc_info, but I'm open to other behavior for that. Not really sure what would help most in CloudWatch Logs Insights queries.

[jaredcnance]

Thanks for the PR! And apologies for the delay in reviewing it. A few comments/questions:

1. Given how Insights currently handles arrays, I wonder if stack_trace would make more sense as a single string. Even when we do improve array query support, I’m not sure what the benefit of having it as an array would be over storing it as a string (other than speculative UX enhancements for arrays, e.g. collapsibility).

2. What’s the purpose of the value property? Having a concrete example would help my understand its use case. Also, doc comments might be helpful.

3. We’ll need to update the README, but we can punt that until the release PR. No need to block this PR.

[jaredcnance]

What’s the use case for allowing a user provided exc_info?

[benkehoe]

I've ended up in situations before where my context changes and the right thing isn't in sys.exc_info() anymore. More generally,I usually default to allowing the user to fully specify the behavior if they want.

[benkehoe]

1. Given how Insights currently handles arrays, I wonder if stack_trace would make more sense as a single string. Even when we do improve array query support, I’m not sure what the benefit of having it as an array would be over storing it as a string (other than speculative UX enhancements for arrays, e.g. collapsibility).

I'm fine with that, especially as each string in the array is multiline anyway (which I wasn't thinking about when I originally thought about the traceback as an array).

2. What’s the purpose of the value property? Having a concrete example would help my understand its use case. Also, doc comments might be helpful.

The idea was to provide some extra user-specified context if needed, and I went for a key, value method API like set_property. For example, logging whatever is happening local to the error handling (for example, was the error remediated?). Maybe it should be a dict for multiple additional fields? Or simply omitted, any context needs to be set in a separate set_property call?

[jaredcnance]

The idea was to provide some extra user-specified context if needed

Maybe “description” would be more appropriate? I think that other context data that I would include should likely be a top-level property (although there may be use cases I can’t think of).

[jaredcnance]

In that case, maybe we just have an optional "details": Any that gives the flexibility for users to embed whatever makes sense for their use case.

[jaredcnance]

JFYI in case you aren't familiar with our release process, this will bake for a couple days in a test environment and then will be released as part of 1.0.3.

[hussam789]

PR Code Suggestions ✨

Category	Suggestion	Impact
General	Improve type annotation specificity The exc_info parameter is typed as Tuple but should be more specific as Tuple[type, BaseException, types.TracebackType] or Tuple[Optional[type], Optional[BaseException], Optional[types.TracebackType]] to match the return type of sys.exc_info(). aws_embedded_metrics/logger/metrics_logger.py [78-82] -def add_stack_trace(self, key: str, details: Any = None, exc_info: Tuple = None) -> "MetricsLogger": +def add_stack_trace(self, key: str, details: Any = None, exc_info: Tuple[type, BaseException, types.TracebackType] = None) -> "MetricsLogger": if not exc_info: exc_info = sys.exc_info() err_cls, err, tb = exc_info Apply this suggestion Suggestion importance[1-10]: 7 __ Why: The suggestion refines the type annotation for the exc_info parameter, improving code clarity and correctness by matching sys.exc_info()'s return type, which is a moderate but valuable enhancement.	Medium
More