-
Notifications
You must be signed in to change notification settings - Fork 90
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
Comments
Steps to Reproduce (for bugs)
|
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? |
I feel like the X-Ray library should validate this. Powertools also has the chance to help customers avoid this problem. |
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. |
@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? |
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 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. |
+1 on the log warn, rather than an exception. it also makes sense to have the same behaviour as python. |
+1 for warning. Runtime exception for monitoring will be a bad user experience. |
Hello - I'll take a look at this :-) |
fixed, will be released after tomorrow |
This is now released under 1.18.0 version! |
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)
Environment
The text was updated successfully, but these errors were encountered: