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

[exporter/awsxray] Adjust AwsXRay segment conversion logic #33000

Conversation

jj22ee
Copy link
Contributor

@jj22ee jj22ee commented May 11, 2024

Description:

Cherry-picking from downstream:
amazon-contributing#111
amazon-contributing#115
amazon-contributing#127

  • We are adjusting the segment creation to accommodate local root spans.
    If a span is a not a local root, then we keep existing behavior.
    If it is a local root then:
    • If it is an Internal or Server span, then promote it to a segment.
      Else we will split it into a segment and subsegment. The segment will represent the service operation and the subsegment will represent the dependency (service A calls service B).
  • Update the common logic for setting segment.Name, which previously only looked at CLIENT/PRODUCER spans, to also look at CONSUMER spans.

Link to tracking Issue:

Testing:
Unit Testing

Documentation:

atshaw43 and others added 4 commits May 10, 2024 20:59
In this commit, we are fixing a couple small bugs missed in the previous review. First, we are adding a nil-guard when settting dependencySubsegment.Name, to avoid the (unlikely, but possible) scenario where local root dependency spans are created without awsRemoteService. Second, we are updating the common logic for setting segment.Name, which previously only looked at CLIENT/PRODUCER spans, but now needs to look at CONSUMER spans.
@jj22ee jj22ee marked this pull request as ready for review May 16, 2024 17:31
jknollmeyer and others added 2 commits May 16, 2024 12:08
* Add logic for stripping the AWS.SDK prefix for Local Root spans

- Previous change to strip the prefix in other cases open-telemetry@7c037ad

* De-duplicate identical logic
Copy link
Contributor

@fatsheep9146 fatsheep9146 left a comment

Choose a reason for hiding this comment

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

small nit

@fatsheep9146 fatsheep9146 added the ready to merge Code review completed; ready to merge by maintainers label May 17, 2024
@andrzej-stencel andrzej-stencel merged commit 902d846 into open-telemetry:main May 22, 2024
162 checks passed
@github-actions github-actions bot added this to the next release milestone May 22, 2024
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this pull request Jul 11, 2024
…metry#33000)

**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
Cherry-picking from downstream:

amazon-contributing#111

amazon-contributing#115

amazon-contributing#127

- We are adjusting the segment creation to accommodate local root spans.
  If a span is a not a local root, then we keep existing behavior.
  If it is a local root then:
    - If it is an Internal or Server span, then promote it to a segment.
Else we will split it into a segment and subsegment. The segment will
represent the service operation and the subsegment will represent the
dependency (service A calls service B).
- Update the common logic for setting segment.Name, which previously
only looked at CLIENT/PRODUCER spans, to also look at CONSUMER spans.

**Link to tracking Issue:** <Issue number if applicable>

**Testing:** <Describe what testing was performed and which tests were
added.>
Unit Testing

**Documentation:** <Describe the documentation added.>

---------

Co-authored-by: atshaw43 <108552302+atshaw43@users.noreply.github.com>
Co-authored-by: Thomas Pierce <thp@amazon.com>
Co-authored-by: John Knollmeyer <jaknollmeyer@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exporter/awsxray ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants