Skip to content

Trace Annotations not getting added if Annotation Key has a space in it #1499

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
thetuxwhocodes opened this issue Oct 31, 2023 · 11 comments
Closed
Assignees
Labels
bug Something isn't working feature-request New feature or request priority:2 High - core feature or affects 60% of the users size/XS

Comments

@thetuxwhocodes
Copy link

What were you trying to accomplish?

I noticed that Trace Annotations not getting added if Annotation Key has a space in it

The following code works
TracingUtils.putAnnotation("TraceName", "MyTrace");
The following code doesn't work
TracingUtils.putAnnotation("Trace Name", "MyTrace");

Expected Behavior

Annotation should be visible in Trace details page

Annotation should be visible in Trace details page irrespective of how the key is created.

Current Behavior

Annotation is totally missing if key has a space in it

Possible Solution

Did not dig deep on the code

Steps to Reproduce (for bugs)

  1. Try with following code.
  2.        TracingUtils.putAnnotation("TraceName", "MyTrace");
    

Environment

  • Powertools for AWS Lambda (Java) version used: 1.17.0
  • Packaging format (Layers, Maven/Gradle): Maven
  • AWS Lambda function runtime: java11
  • Debugging logs

How to enable debug mode**

# paste logs here
@thetuxwhocodes
Copy link
Author

Steps to Reproduce (for bugs)

Try with following code.

       TracingUtils.putAnnotation("Trace Name", "MyTrace");

@scottgerring
Copy link
Contributor

Hey @thetuxwhocodes , thanks for raising the issue!

It looks like annotation keys have to be alphanumeric without spaces for X-Ray. I'm not sure what we should do here - the underlying x-ray library isn't validating this - should we? What do you think?

@msailes
Copy link
Contributor

msailes commented Nov 2, 2023

I feel like the X-Ray library should validate this. Powertools also has the chance to help customers avoid this problem.

@thetuxwhocodes
Copy link
Author

The confusion arises when space is allowed for metadata key and not for annotations. If currently space cannot be supported, I would suggest updating the powertools documentation to make it clear.

@scottgerring
Copy link
Contributor

@msailes ACK your point about improving user XP. I feel like it belongs primarily in x-ray and will raise a ticket over there also.

If PT were to validate it, the result would likely be a runtime exception. At least then you'd have a some immediate indication of the problem, rather than a silent failure and a bug hunt. What do you both think about this?

@leandrodamascena
Copy link

leandrodamascena commented Nov 7, 2023

Hey Java folks!!

The X-Ray SDK for Python emits a warning if some unsupported characters are found - https://github.com/aws/aws-xray-sdk-python/blob/9e6fa9f5aed3e36ec1590172f296a2c2c8666608/aws_xray_sdk/core/models/entity.py#L151

If PT were to validate it, the result would likely be a runtime exception. At least then you'd have some immediate indication of the problem, rather than a silent failure and a bug hunt. What do you both think about this?

If the X-Ray SDK for Java doesn't address this, I completely agree that Powertools should improve this experience. But I don't think this should raise a runtime exception. In general, I believe we shouldn't raise a Runtime error when talking about observability. It is better to emit a warning so that the customer catches this warning and can work to fix the problem with the minimum of data loss - just an annotation in this case.

@jeromevdl
Copy link
Contributor

+1 on the log warn, rather than an exception. it also makes sense to have the same behaviour as python.

@thetuxwhocodes
Copy link
Author

+1 for warning. Runtime exception for monitoring will be a bad user experience.

@jeromevdl jeromevdl added feature-request New feature or request size/XS priority:2 High - core feature or affects 60% of the users and removed triage labels Nov 8, 2023
@jdoherty
Copy link
Contributor

jdoherty commented Nov 8, 2023

Hello - I'll take a look at this :-)

@jeromevdl
Copy link
Contributor

fixed, will be released after tomorrow

@scottgerring scottgerring moved this from Working on it to Coming soon in Powertools for AWS Lambda (Java) Nov 15, 2023
Copy link
Contributor

This is now released under 1.18.0 version!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feature-request New feature or request priority:2 High - core feature or affects 60% of the users size/XS
Projects
Status: Shipped
Development

No branches or pull requests

6 participants