Skip to content

fix: spans should only include path in http.route#7390

Merged
bnjjj merged 5 commits intodevfrom
caroline/telemetry-http-route
May 6, 2025
Merged

fix: spans should only include path in http.route#7390
bnjjj merged 5 commits intodevfrom
caroline/telemetry-http-route

Conversation

@carodewig
Copy link
Contributor

@carodewig carodewig commented May 2, 2025

Per the OpenTelemetry spec, the http.route should only include "the matched route, that is, the path template used in the format used by the respective server framework."

The router currently sends the full URI in http.route, which can be high cardinality (ie /graphql?operation=one_of_many_values). After this change, the router will only include the path (/graphql).

In order to test this, I added a dependency on tracing-mock. It's currently in beta, but it's already used within the tracing library itself so I think it's probably pretty stable. However I'm happy to come up with another way to test this if preferred.

Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

  • Changes are compatible1
  • Documentation2 completed
  • Performance impact assessed and acceptable
  • Tests added and passing3
    • Unit Tests
    • Integration Tests
    • Manual Tests

Exceptions

Note any exceptions here

Notes

Footnotes

  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. ↩

@github-actions

This comment has been minimized.

Sign in to view
@svc-apollo-docs
Copy link
Collaborator

svc-apollo-docs commented May 2, 2025 •
edited
Loading

✅ Docs preview has no changes

The preview was not built because there were no changes.

Build ID: 99c7abda36d41f1bf38e5472

@carodewig carodewig changed the title fix: limit http.route to just the path in telemetry fix: spans should only include path in http.route May 2, 2025
@carodewig carodewig marked this pull request as ready for review May 2, 2025 19:57
@carodewig carodewig requested review from a team as code owners May 2, 2025 19:57
@carodewig carodewig added the backport-1.x Backport this PR to 1.x label May 2, 2025
@carodewig carodewig requested a review from bnjjj May 5, 2025 14:36
bnjjj
bnjjj approved these changes May 5, 2025
View reviewed changes
Copy link
Contributor

@bnjjj bnjjj left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Do we have a way to provide the full uri ? An existing selector to make sure if our users would like to have this data they can still have it ?

@carodewig
Copy link
Contributor Author

carodewig commented May 5, 2025

Do we have a way to provide the full uri ? An existing selector to make sure if our users would like to have this data they can still have it ?

@bnjjj Good question - it looks like we can provide the URL path and query via instrumentation attributes: https://www.apollographql.com/docs/graphos/routing/observability/telemetry/instrumentation/standard-attributes#router

@bnjjj bnjjj merged commit 46ae381 into dev May 6, 2025
15 checks passed
@bnjjj bnjjj deleted the caroline/telemetry-http-route branch May 6, 2025 10:37
carodewig added a commit that referenced this pull request May 6, 2025
@carodewig
fix: spans should only include path in http.route (#7390)
426ce17 
(cherry picked from commit 46ae381)
@carodewig carodewig mentioned this pull request May 6, 2025
Merged
6 tasks
abernix pushed a commit that referenced this pull request May 6, 2025
@carodewig @abernix
fix: spans should only include path in http.route (#7390)
7dfcb35
@abernix abernix mentioned this pull request May 13, 2025
Merged
carodewig added a commit that referenced this pull request May 27, 2025
@carodewig
fix: spans should only include path in http.route (backport #7390) (#…
85a4580 
…7405)
@lrlna lrlna mentioned this pull request Jun 2, 2025
Merged
Aguilarjaf added a commit to ExpediaGroup/apollo-router that referenced this pull request Jun 9, 2025
@Aguilarjaf @dylan-apollo
@conwuegb @abernix @smyrick @lleadbet @shorgi @carodewig @goto-bus-stop @duckki @clenfest @naomijub @renovate @BrynCooke @bnjjj @lennyburdette @lrlna @mergify @sachindshinde @tninesling @sam-friedman-apollo @DaleSeo @benjamn @dariuszkuc @IvanGoncharov
v2.2.1 (#31)
721100f 
* Remove duplicate reqwest (apollographql#7329)

* Port first batch of supgraph validation tests (apollographql#7321)

- Adds a portion of the subgraph validation tests being ported over from the federation repo.
- Exposes error fields on SubgraphError and FederationError for use in the tests.
  - Creates a public getter method for SubgraphError's error field
  - Creates a public getter method returning an array of errors associated with a given FederationError
- Ports over the build_for_errors() test utility.

* chore(deps): update xtask/Cargo.lock h2 dependency (apollographql#7336)

* [docs] Add new page for query planning best practices (apollographql#7263)

Co-authored-by: Lucas Leadbetter <5595530+lleadbet@users.noreply.github.com>
Co-authored-by: Edward Huang <edhuang@gmail.com>

* fix: terminate unused Redis connections on schema update (apollographql#7319)

Co-authored-by: Renée <renee.kooi@apollographql.com>

* test(federation/composition): ported the rest of "@core/@link handling" tests  (apollographql#7335)

* Add code for add_shareable and has_applied_directive (apollographql#7327)

* Basic changes to connect/

* docs: otlp metrics exporters (apollographql#7334)

* Removes redundant clones

* add const fn warning

* Remove const_fn warning

* Apply comments

* Remove todo comment

* Update apollo-federation/src/sources/connect/json_selection/parser.rs

Co-authored-by: Dylan Anthony <dylan@apollographql.com>

* Update apollo-federation/src/sources/connect/json_selection/parser.rs

Co-authored-by: Dylan Anthony <dylan@apollographql.com>

* Reformats code after suggestions

* fix(deps): update rust crate thiserror to v2 (apollographql#7288)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Renée Kooi <renee.kooi@apollographql.com>

* deps: update apollo-compiler (apollographql#7338)

* Add _dd.measured to spans when agent sampling is enabled (apollographql#7344)

Co-authored-by: bryn <bryn@apollographql.com>

* Fix metric `apollo.router.operations.batching.size` (apollographql#7306)

Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com>
Co-authored-by: Jesse Rosenberger <git@jro.cc>

* Clarify coprocessor config `context: true` behaviour (apollographql#7349)

* Avoid unnecessary cloning in the deduplication plugin (apollographql#7347)

* expanded supergraph now uses join spec v0.5 so that we don't skip the… (apollographql#7351)

* make use of new build image when building the router (apollographql#7355)

* Move content negotiation to a plugin (apollographql#7316)

Co-authored-by: Iryna Shestak <shestak.irina@gmail.com>

* Port batch of `subgraph_validation_tests` from federation repo (apollographql#7345)

* DOC-391 clarify tracing exporters configs of OTLP

* edit overview

* Add subgraph validation for interface objects (apollographql#7317)

Port `validateKeyOnInterfacesAreAlsoOnAllImplementations()` and `validateInterfaceObjectsAreOnEntities()` from the JS composition code, i.e. the per-subgraph validations for interface objects. Note that as part of this, `printHumanReadableList()` and `joinStrings()`, were also ported.

* rewrite upgrade_subgraphs_if_necessary() to minimize cloning and fix tests. (apollographql#7346)

* Update schema upgrader to use apollo-compiler 1.28 (apollographql#7356)

* test(federation/composition): Port subgraph validation tests (part 2) (apollographql#7343)

Co-authored-by: Taylor Ninesling <taylor.ninesling@apollographql.com>

* Fix clippy when run with `--all-features` (apollographql#7352)

* Move aggressive lints to connectors module

* temporarily ignore failing tests in dev (apollographql#7375)

* fix-less-performant-code

* docs: fix typo in distributed caching (apollographql#7358)

* docs: typo on PQ docs (apollographql#7373)

* Fix router enforcement of connect spec version. (apollographql#7377)

* Add a test in connectors batching for handling gaps (apollographql#7328)

* Remove fed1 links and reexpand after fixing federation arguments (apollographql#7374)

* DOC-448 clarify apollo_router_session_count_active removal

* feat(federation/composition): ported `@external` directive validation (apollographql#7381)

* Remove 0.2 version checks in composition for headers & connect-on-type (apollographql#7387)

* feat(federation/composition): implemented `UNSUPPORTED_ON_INTERFACE` errors (apollographql#7382)

* implement ->joinNotNull method (apollographql#7165)

Co-authored-by: Ben Newman <ben@apollographql.com>

* chore: remove dead code (apollographql#7391)

* Auto-upgrade connect 0.1 subgraphs to 0.2 in composition (apollographql#7388)

* telemetry: remove excessive bool from DisplayRouterResponse(refactoring) (apollographql#7331)

* Simplify condition handling in coprocessor (apollographql#7320)

* Implement demand control directive validations (apollographql#7376)

* Remove aws-lc from dependencies (apollographql#7389)

* Account for spec aliases/imports when auto-injecting `link` spec directives/types (apollographql#7365)

When the `@link` directive definition is missing, we have some logic that automatically adds the `link` spec's type/directive definitions to the schema. We weren't accounting for the link spec's aliases/imports when doing this, while the JS codebase was, so we've ported that logic here. Note that this logic has several bugs, but we're going to port it as-is for now and address the bugs later.

* chore(federation/composition): deleted a duplicate test module `link_handling_tests` (apollographql#7392)

* fix: decrease log level for JWT authentication failure (apollographql#7396)

* fix: make sure all input types end up in the expanded supergraph (apollographql#7383)

* remove apollographql#7383 changeset (apollographql#7401)

* Revert "Add _dd.measured to spans when agent sampling is enabled" (apollographql#7403)

* fix: spans should only include path in `http.route` (apollographql#7390)

* fix: propagate client name and version modifications through telemetry (apollographql#7369)

* Manage all CI versions through mise and use binstall (backport apollographql#7393) (apollographql#7410)

* prep release: v2.2.1-rc.0

* macos specific mise: we need x86_64-apple-darwin + aarch64-apple-darwin

these both need to be installed on our releasing image

* Remove no-longer-necessary installation of `gh` via Orb ✨

This is now accomplished with `gh`

* prep release: v2.2.1-rc.1

* Include inadvertently removed fetch_dependencies

This was inadverently removed in apollographql#7410 but should continue to exist.

* prep release: v2.2.1

---------

Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com>
Co-authored-by: Dylan Anthony <dylan@apollographql.com>
Co-authored-by: Chidimma O <conwuegb@users.noreply.github.com>
Co-authored-by: Jesse Rosenberger <git@jro.cc>
Co-authored-by: Shane Myrick <mail@shanemyrick.com>
Co-authored-by: Lucas Leadbetter <5595530+lleadbet@users.noreply.github.com>
Co-authored-by: Edward Huang <edhuang@gmail.com>
Co-authored-by: Caroline Rodewig <16093297+carodewig@users.noreply.github.com>
Co-authored-by: Renée <renee.kooi@apollographql.com>
Co-authored-by: Duckki Oe <duckki.oe@apollographql.com>
Co-authored-by: Chris Lenfest <clenfest@apollographql.com>
Co-authored-by: Julia Naomi <jnboeira@outlook.com>
Co-authored-by: Edward Huang <edward.huang@apollographql.com>
Co-authored-by: Julia Naomi <naomijub@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Bryn Cooke <BrynCooke@gmail.com>
Co-authored-by: bryn <bryn@apollographql.com>
Co-authored-by: Coenen Benjamin <benjamin.coenen@hotmail.com>
Co-authored-by: Gary Pennington <gary@apollographql.com>
Co-authored-by: Lenny Burdette <lenny@apollographql.com>
Co-authored-by: Iryna Shestak <shestak.irina@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Sachin D. Shinde <sachin@apollographql.com>
Co-authored-by: Taylor Ninesling <taylor.ninesling@apollographql.com>
Co-authored-by: Sam Friedman <sam.friedman@apollographql.com>
Co-authored-by: Dale Seo <5466341+DaleSeo@users.noreply.github.com>
Co-authored-by: Ben Newman <ben@apollographql.com>
Co-authored-by: Dariusz Kuc <9501705+dariuszkuc@users.noreply.github.com>
Co-authored-by: Ivan Goncharov <ivan.goncharov.ua@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@bnjjj bnjjj bnjjj approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backport-1.x Backport this PR to 1.x

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@carodewig @svc-apollo-docs @bnjjj