Bug: Generates valid subsegment names that comply with X-Ray SDK restrictions

[loesak]

Expected Behaviour

When decorating a method with tracer.capture_method, it should generate names that are compatible with the AWS X-Ray Python SDK.

Current Behaviour

In some circumstances, it generates subsegment names that are incompatible with the AWS X-Ray Python SDK. This will continually cause AWS X-Ray Python SDK to produce a WARNING log message.

Code snippet

def get_inner_func():
    @tracer.capture_method
    def inner_func():
        pass

    return inner_func

if __name__ == "__main__":
    method = get_inner_func()
    print(f"{method.__module__}.{method.__qualname__}")

OUTPUTS:

__main__.get_inner_func.<locals>.inner_func

which is an invalid name for a segment and generates a warning.

Possible Solution

The name of the subsegment is taken from the method name using method_name = f"{method.module}.{method.qualname}" but does nothing to sanitize the result to make it compatible with AWS X-Ray or AWS X-Ray Python SDK.

AWS X-Ray docs say the name can be:

The logical name of the service that handled the request, up to 200 characters. For example, your application's name or domain name. Names can contain Unicode letters, numbers, and whitespace, and the following symbols: _, ., :, /, %, &, #, =, +, \, -, @

AWS X-Ray Python SDK strips the following characters:

_common_invalid_name_characters = '?;*()!$~^<>'

From PEP 3155 - Qualified name for classes and functions

This PEP proposes the addition of a qualname attribute to functions and classes. For top-level functions and classes, the qualname attribute is equal to the name attribute. For nested classes, methods, and nested functions, the qualname attribute contains a dotted path leading to the object from the module top-level. A function’s local namespace is represented in that dotted path by a component named <locals>.

In our case, our segment is being affected by "A function’s local namespace is represented in that dotted path by a component named <locals>." where the "<" and ">" are invalid characters and raising a WARNING in our logs.

Add logic to sanitize the value of method_name to be compatible with AWS X-Ray or AWS X-Ray Python SDK by restricting the length to 200 characters and removing any invalid character.

Steps to Reproduce

Create a locally defined function and decorate it with @tracer.capture_method and receive WARNING in logs.

Powertools for AWS Lambda (Python) version

2.26.0

AWS Lambda function runtime

3.11

Packaging format used

PyPi

Debugging logs

No response

[boring-cyborg]

Thanks for opening your first issue here! We'll come back to you as soon as we can.
In the meantime, check out the #python channel on our Powertools for AWS Lambda Discord: Invite link

[leandrodamascena]

Hi @loesak, thank you for bringing up this concern. Although I agree that we should ensure the generated subsegment names comply with the X-Ray specification, I wouldn't classify this as a bug since there's no loss of tracer or subsegment data. It's more of a warning propagated by the X-Ray SDK.

I believe it's a good idea to add a sanitization method before creating the subsegment name. However, I would like to have opinions from other maintainers before making a final decision. @sthulb @rubenfonseca

I'm reclassifying this issue as a feature request and adding labels until we reach a decision.

Thank you

[sthulb]

Hi folks! I like this idea – I think adding sanitisation on things we generate to ensure it works as expected is a good idea.

Are there any known issues with backwards compat. with this?

[am29d]

The name is mostly relevant for the developer to understand the call structure and troubleshoot. I don't see the internals such as <locals> as a relevant, the chain is remains readable without it. I wonder what other cases we don't know when changing the method name would make it more difficult to understand the cause.

But overall, sanitising the method name according to X-Ray requirements is the right step.

[rubenfonseca]

Agree, sanitising the method name can't hurt

[leandrodamascena]

Thanks @am29d and @sthulb! I agree that removing these invalid characters - _common_invalid_name_characters = '?;*()!$~^<>' - is the right approach to ensure compliance with the X-Ray SDK.

Are there any known issues with backwards compat. with this?

I don't see any backwards compact @sthulb. X-Ray SDK already strips these characters from the subsegment name and generates a warning.

The name is mostly relevant for the developer to understand the call structure and troubleshoot. I don't see the internals such as as a relevant, the chain is remains readable without it. I wonder what other cases we don't know when changing the method name would make it more difficult to understand the cause.

I agree that <locals> or even locals don't contribute to improving the readability of the subsegment name. While I'd like to remove it, it might be a breaking change for customers who rely on this name. Additionally, locals is not a reserved word in Python, so removing it could have unintended side effects.

Hey @loesak, would you like to send a PR to sanitize the name before creating the subsegment? Your contribution would be greatly appreciated here!

Thank you for your insights, @rubenfonseca, @am29d and @sthulb!

[leandrodamascena]

Returning with some concerning findings that I'd like to discuss. Upon reviewing our tests, I've noticed that we're testing scenarios such as nested context managers, where the name may contain characters like <.

While I agree that it's essential to avoid allowing these characters and we must sanitize them before sending to XRay SDK, I'm hesitant about potentially introducing a breaking change in our customers' CI pipelines.

I'm considering adding labels to Powertools V3, with modifications planned for a major release. What do you think @am29d @rubenfonseca @sthulb?

[rubenfonseca]

Nice customer obsession Leandro, well done! Normally I would agree that it could be a breaking change. However, consider the following:

• what we are trying to do here is to fix a bad bahviour, since the current code is producing invalid names
• in our docs we even recommend to not test the Tracer utility anyway

With those things in mind, I would still consider this to be a bug fix rather than a breaking change

[leandrodamascena]

Thank you for your concise and insightful points, @rubenfonseca.

Sending a PR right now.

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

This issue is now closed. Please be mindful that future comments are hard for our team to see.

If you need more assistance, please either tag a team member or open a new issue that references this one.

If you wish to keep having a conversation with other community members under this issue feel free to do so.

[loesak]

Thank you for the quick turn around @leandrodamascena . Much appreciated.

[github-actions]

This is now released under 2.36.0 version!