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.

Sign up for GitHub

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

Jump to bottom

add configuration for trace_id #2131

Merged
merged 15 commits into from
Nov 24, 2022
Merged

add configuration for trace_id #2131

merged 15 commits into from
Nov 24, 2022

Conversation

bnjjj
Copy link
Contributor

@bnjjj bnjjj commented Nov 18, 2022 •
edited
Loading

close #2080

Configuration example:

telemetry:
  tracing:
    experimental_response_trace_id:
      enabled: true # default: false
      header_name: "my-trace-id" # default: "apollo-trace-id"
    propagation:
      request: 
        header_name: "x-request-id"
      jaeger: true

TODO:

  • Tests
  • Enable it in dev mode
  • docs

bnjjj added 2 commits November 18, 2022 16:35
@bnjjj
add support of experimental_expose_trace_id
5577916 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
add support of custom header propagator for trace_id
0d601ac 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj bnjjj self-assigned this Nov 18, 2022
@bnjjj bnjjj requested review from BrynCooke, Geal and garypen and removed request for Geal November 18, 2022 17:11
@github-actions

This comment has been minimized.

Sign in to view
@bnjjj bnjjj changed the title Bnjjj/feat 2080 add configuration for trace_id Nov 18, 2022
@bnjjj
add docs
b136e0d 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj bnjjj requested a review from StephenBarlow as a code owner November 21, 2022 16:07
bnjjj added 4 commits November 21, 2022 17:14
@bnjjj
Merge branch 'dev' of github.com:apollographql/router into bnjjj/feat…
94c2571 
…_2080
@bnjjj
fix tests
2130697 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
changelog
79ed333 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
Merge branch 'dev' of github.com:apollographql/router into bnjjj/feat…
181672d 
…_2080
Geal
Geal reviewed Nov 23, 2022
View reviewed changes
apollo-router/src/plugins/telemetry/mod.rs Outdated Show resolved Hide resolved
NEXT_CHANGELOG.md Outdated Show resolved Hide resolved
@bnjjj
Update apollo-router/src/plugins/telemetry/mod.rs
77cecff 
Co-authored-by: Geoffroy Couprie <geoffroy@apollographql.com>
abernix
abernix reviewed Nov 23, 2022
View reviewed changes
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.

Couple small comments!

...uter/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap Outdated Show resolved Hide resolved
NEXT_CHANGELOG.md Outdated Show resolved Hide resolved
NEXT_CHANGELOG.md Outdated Show resolved Hide resolved
bnjjj added 3 commits November 23, 2022 15:05
@bnjjj
change naming
3effab5 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
fix test
fad11d2 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
Merge branch 'dev' of github.com:apollographql/router into bnjjj/feat…
803c0a2 
…_2080
@bnjjj bnjjj requested review from abernix and Geal November 23, 2022 15:05
@bnjjj
add disclaimer on experimental feature
b687058 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj bnjjj added the experimental Related to our experimental features/configurations label Nov 23, 2022
@bnjjj
change configuration shape
bf5ff08 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
BrynCooke
BrynCooke approved these changes Nov 24, 2022
View reviewed changes
Copy link
Contributor

@BrynCooke BrynCooke left a comment

Choose a reason for hiding this comment

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

Approved with the following comments.

  1. Not sure we should at this stage bother populating the response headers in the ApolloExporter. It's potentially inaccurate right now, and we'll need to think about how we configure this properly.

  2. Still not keen on the enabled thing. Would prefer that the value "default" is used under the header_name. There's no hard and fast rules on when to use enabled, but I guess we can tackle this when we move out of experimental.

garypen
garypen approved these changes Nov 24, 2022
View reviewed changes
Copy link
Contributor

@garypen garypen left a comment

Choose a reason for hiding this comment

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

Looks great. One question that I'd like to know the answer to.

apollo-router/src/axum_factory/utils.rs Show resolved Hide resolved
o0Ignition0o
o0Ignition0o approved these changes Nov 24, 2022
View reviewed changes
Copy link
Contributor

@o0Ignition0o o0Ignition0o left a comment

Choose a reason for hiding this comment

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

LGTM overall, a couple of nits that seem to be copy paste mishaps.

Love the experimental documentation section, this is great! ❤️

apollo-router/tests/common.rs Outdated Show resolved Hide resolved
docs/source/configuration/tracing.mdx Outdated Show resolved Hide resolved
@bnjjj
undo formatting in tracing.mdx
7e691ef 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
Copy link
Contributor Author

bnjjj commented Nov 24, 2022

@BrynCooke

Approved with the following comments.

  1. Not sure we should at this stage bother populating the response headers in the ApolloExporter. It's potentially inaccurate right now, and we'll need to think about how we configure this properly.

We agreed with the studio team to use this as a workaround right now, they have an issue on their side to add a field in proto file to handle that correctly. It's still correct except we don't insert all response headers inside. We discussed with Jesse about that and if we see a need or request from our users we will add them all later but for now that's ok.

  1. Still not keen on the enabled thing. Would prefer that the value "default" is used under the header_name. There's no hard and fast rules on when to use enabled, but I guess we can tackle this when we move out of experimental.

Could you add this comment in the right discussion please ? --> #2147

@bnjjj bnjjj merged commit 2039105 into dev Nov 24, 2022
@bnjjj bnjjj deleted the bnjjj/feat_2080 branch November 24, 2022 16:40
garypen pushed a commit that referenced this pull request Nov 30, 2022
@bnjjj @garypen
add configuration for trace_id (#2131)
b9431ed 
close #2080 

Configuration example:

```yaml
telemetry:
  tracing:
    experimental_response_trace_id:
      enabled: true # default: false
      header_name: "my-trace-id" # default: "apollo-trace-id"
    propagation:
      request: 
        header_name: "x-request-id"
      jaeger: true
```

Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
Co-authored-by: Geoffroy Couprie <geoffroy@apollographql.com>
@BrynCooke BrynCooke added this to the v1.5.0 milestone Dec 3, 2022
@garypen garypen added this to the v1.5.0 milestone Dec 5, 2022
@BrynCooke BrynCooke modified the milestone: v1.5.0 Dec 5, 2022
@BrynCooke BrynCooke mentioned this pull request Dec 5, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@o0Ignition0o o0Ignition0o o0Ignition0o approved these changes

@BrynCooke BrynCooke BrynCooke approved these changes

@garypen garypen garypen approved these changes

@StephenBarlow StephenBarlow Awaiting requested review from StephenBarlow

@abernix abernix Awaiting requested review from abernix

@Geal Geal Awaiting requested review from Geal

Assignees

@bnjjj bnjjj

Labels
experimental Related to our experimental features/configurations
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add configuration for trace_id
6 participants
@bnjjj @garypen @Geal @BrynCooke @abernix @o0Ignition0o