Skip to content
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

Fixed filtered attribute translation in the AWS X-Ray exporter. #3757

Merged
merged 4 commits into from
Jun 16, 2021

Conversation

jefchien
Copy link
Contributor

Description: In the AWS X-Ray exporter, non-string attribute values were showing up as empty strings. Imported the AttributeValueToString function from protospan_translation.go and used it to perform the translation.

Link to tracking Issue: aws-observability/aws-otel-collector#518

Testing: Tested with aws-otel-test-framework. Added TestSpanWithFilteredAttributes test to http_test.go to check each of the types.

@jefchien jefchien requested a review from a team June 11, 2021 23:25
@anuraaga
Copy link
Contributor

Thanks for looking into this @jefchien. It looks like there is a fundamental problem with the attribute filtering - it should still use attributes, not strings, since XRay supports non-string attributes. We convert here

https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/exporter/awsxrayexporter/translator/segment.go#L327

But currently only do so for resources attributes - the same should be done for span attributes. Would you be able to make the change so the attribute filtering happens on attributes instead of string map?

@jefchien
Copy link
Contributor Author

So, instead of converting the pdata.AttributeValues to strings in http.go, the filtered span attributes should be passed in full until they reach the metadata or are otherwise used. That makes sense. Thanks for the clarification!

…type throughout segment generation.

Moved test from http_test.go to segment_test.go
Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@bogdandrutu bogdandrutu merged commit 74d314c into open-telemetry:main Jun 16, 2021
@jefchien jefchien deleted the 518-non-string-span branch January 18, 2023 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants