Consider using warnings over logging.warning for important messages

[heitorlessa]

Hello X-Ray team!

I've recently discovered that tracing annotations with - or spaces in the name are silently dropped at the SDK level. When looking for a possible cause, I've found out X-Ray documentation calls this out in the API Segment Document schema page.

It would've been quicker to spot this issue if X-Ray SDK were to use the standard library Warnings package. This allows library maintainers to warn their users and give them a chance to suppress certain filters at will.

What's the difference between logging.warning vs warnings.warn?

For Logging, it's a good practice for library owners to use a NullHandler. This means any logging message produced by a library will be silently dropped, unless the consumer explicitly enables the library logging in question (or sets a Root logger).

In contrast, warnings.warn will notify consumers that something it's worth investigating. They also support categories of warnings to allow complex libraries to have multiple warnings, so their consumers can explicitly disable a subset of warnings without having to know every possible message to filter out.

Here's an example in Lambda Powertools for Python where we warn customers if they have no metrics to be flushed. This will happen regardless of how they configure their loggers. If they're intentional about having no user-defined metrics, they suppress this warning altogether with warnings.filterwarnings("ignore", "No metrics to publish*").

Hopefully this will help strike a balance between not interrupting customers at runtime while warning them they might be losing important tracing information.

Thank you!

[wangzlei]

Thanks for raise this issue, I incline toward your idea.

In library design there is always a trade-off between silently drop and actively call out if the issue happens. One principle in my mind is if the problem is caused by customer, a good practice is by default library raises a clear signal, in java is runtime exception-like. Whether ignore it, print it or fast-fail is up to user.

It seems to get worse if library worries too much about interrupting customer to point the issue happens. warning.warn sounds a better option. We will have further discussion for this suggestion. Since currently the code is using logging, one concern is how to avoid customer impact for existing customers, in case of flooding their application log.

[heitorlessa]

Glad to hear it! I understand your concern about flooding their application. As a library author, I too pondered about it when implementing in Lambda Powertools last year; please allow me to share another angle.

Customers would only see these warnings in their logs if they are using unsupported values. This allows customers to respond in two ways: 1/ Correct their values to prevent data loss in their observability strategy, or 2/ Intentionally disable these warnings as these might be test data to cause this wanted outcome.

[NathanielRN]

Thanks for raising this issue @heitorlessa! We reviewed this issue with our team internally and your explanation made a lot of sense to all of us. We especially appreciate that it will be a backwards-compatible change that wouldn't require a major version bump.

With that being said, would you be willing to open a PR to add a warnings.warn message for this case of invalid annotation keys? We'd be happy to follow up with a review and get it merged 🙂

Thanks again for your help!

[heitorlessa]

hey, absolutely. Handling a few issues in Lambda Powertools until the end of 1st week of August, and I can send a PR for that section only.