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

Add Support for AWS X-Ray Tracing Propagator #3580

Merged
merged 5 commits into from
Sep 13, 2023

Conversation

scottmace
Copy link
Contributor

Adds support for the OpenTelemetry AWS X-Ray tracing propagator.

This propagator helps propagate tracing information from upstream services (such as AWS load balancers) to downstream services and handles conversion between the X-Ray trace id format and OpenTelemetry span contexts.

Checklist

Complete the checklist (and note appropriate exceptions) before a final PR is raised.

  • Changes are compatible[^1]
  • Documentation[^2] completed
  • Performance impact assessed and acceptable
  • Tests added and passing[^3]
    • Unit Tests
    • Integration Tests
    • Manual Tests

Exceptions

Note any exceptions here

Notes

[^1]. It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this.
[^2]. Configuration is an important part of many changes. Where applicable please try to document configuration examples.
[^3]. Tick whichever testing boxes are applicable. If you are adding Manual Tests:
- please document the manual testing (extensively) in the Exceptions.
- please raise a separate issue to automate the test and label it (or ask for it to be labeled) as manual test

@scottmace scottmace requested a review from a team as a code owner August 15, 2023 18:32
Copy link
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

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

Thanks for opening this. I actually came across another issue about this today and finally got around to leaving my comment on it. Maybe you could respond there? #3445 (comment)

Other than that, one comment within here, though as my question on the other issue will elude to, I'm curious if this could just be done with the existing "custom header" option that exists and how this differs, in practice.

Thanks again!

@@ -81,6 +81,9 @@ telemetry:
# https://zipkin.io/ (compliant with opentracing)
zipkin: false

# https://aws.amazon.com/xray/ (compliant with opentracing)
Copy link
Member

Choose a reason for hiding this comment

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

(compliant with opentracing)

I'm actually curious what this comment means (even on the other options). Is it compatible? What makes it so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that It means that the format is compatible with the OpenTelemetry OTLP exporter/collector.

Copy link
Member

@abernix abernix Aug 16, 2023

Choose a reason for hiding this comment

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

"OpenTelemetry" makes sense, but I was more curious about "OpenTracing" (which is different, as I understand it).

However, your uncertainty here is totally fine. I only asked because I thought maybe you'd perhaps made understanding of it already. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yeah. It looks like OpenTracing was merged into OpenTelemetry. Perhaps the comments should be updated.

@apollo-cla
Copy link

@scottmace: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@router-perf
Copy link

router-perf bot commented Aug 15, 2023

CI performance tests

  • xxlarge-request - Stress test with 100 MB request payload
  • step - Basic stress test that steps up the number of users over time
  • events_without_dedup - Stress test for events with a lot of users and deduplication DISABLED
  • events - Stress test for events with a lot of users and deduplication ENABLED
  • large-request - Stress test with a 1 MB request payload
  • const - Basic stress test that runs with a constant number of users
  • no-graphos - Basic stress test, no GraphOS.
  • reload - Reload test over a long period of time at a constant rate of users
  • step-jemalloc-tuning - Clone of the basic stress test for jemalloc tuning
  • xlarge-request - Stress test with 10 MB request payload
  • events_big_cap_high_rate - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity

@abernix
Copy link
Member

abernix commented Aug 16, 2023

Note that I think the tests are failing because of the need to commit the changes after reviewing snapshot changes with cargo insta test. (You'd have to cargo install cargo-insta first, to use cargo insta test)

@bnjjj
Copy link
Contributor

bnjjj commented Sep 12, 2023

@scottmace Thanks ! It looks good! Sorry for the delay, could you generate a small changeset for that pr ? Using cargo xtask changeset create please

@bnjjj bnjjj merged commit cc98b64 into apollographql:dev Sep 13, 2023
1 check passed
johnnywalker pushed a commit to dfh-foundation/router that referenced this pull request Sep 21, 2023
Adds support for the OpenTelemetry AWS X-Ray tracing
[propagator](https://docs.rs/opentelemetry-aws/latest/opentelemetry_aws/).

This propagator helps propagate tracing information from upstream
services (such as AWS load balancers) to downstream services and handles
conversion between the X-Ray trace id format and OpenTelemetry span
contexts.
@lrlna lrlna mentioned this pull request Sep 27, 2023
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.

5 participants