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

Federated tracing #2479

Merged
merged 34 commits into from
Dec 7, 2023
Merged

Federated tracing #2479

merged 34 commits into from
Dec 7, 2023

Conversation

cappuc
Copy link
Contributor

@cappuc cappuc commented Nov 29, 2023

Resolves #2053
Resolves #2068

  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

Changes

This PR adds support for Apollo Federated tracing.

I used the apollo server implementation as reference.

Error reporting

Federated tracing spec supports errors reporting but we need to decide how to implement this feature in order to prevent exposing sensitive data (without user control), right now there is no setting for this.

Apollo server has this options:

  • mask error (default)
  • include the full error
  • transform the error with a callback

Here the docs and implementation

Breaking changes

None. the old tracing implementation is enabled by default.
This new implementation must be manually enabled through config.

Some notes

The new tracing spec uses protobuf and the updated of the proto file is manual:

  • copy the reports.proto file from the apollo-server repo
  • remove the js specific directives (for example: [(js_use_toArray)=true] and [(js_preEncoded)=true]
  • add the php options:
    option php_namespace = "Nuwave\\Lighthouse\\Tracing\\FederatedTracing\\Proto";
    option php_metadata_namespace = "Nuwave\\Lighthouse\\Tracing\\FederatedTracing\\Proto\\Metadata";

Then run make:proto and make:fix to generate (and fix sytle) the protobuf classes

composer.json Outdated Show resolved Hide resolved
src/Events/BuildExtensionsResponse.php Outdated Show resolved Hide resolved
src/Tracing/ApolloTracing/ApolloTracing.php Outdated Show resolved Hide resolved
src/Tracing/FederatedTracing/FederatedTracing.php Outdated Show resolved Hide resolved
@cappuc
Copy link
Contributor Author

cappuc commented Dec 1, 2023

I've extracted the ftv1 string to a constant and replaced it inside the driver and in the headers value of the tests.

I'm not sure it is a good idea to replace it in the test assertions too because it should be replaced event in calls like $response->json('extensions.ftv1') if we want to always use the constant and I think it is worst for readability.

I also renamed the value of the config to federated-tracing so it is not confused with the header/extension value.

composer.json Outdated Show resolved Hide resolved
src/Tracing/TracingServiceProvider.php Outdated Show resolved Hide resolved
src/lighthouse.php Outdated Show resolved Hide resolved
@spawnia
Copy link
Collaborator

spawnia commented Dec 1, 2023

Let's document the process for generating/updating the proto files in CONTRIBUTING.md at least. Preferrably, I would like to automate the process through GitHub actions.

.github/workflows/proto.yml Outdated Show resolved Hide resolved
buf.gen.yaml Show resolved Hide resolved
buf.gen.yaml Outdated Show resolved Hide resolved
rector.php Outdated Show resolved Hide resolved
@cappuc
Copy link
Contributor Author

cappuc commented Dec 4, 2023

The workflow should work but I couldn't get it to work on pull request (the main problem is the commit part).
I enabled it only on push like the format workflow

buf.gen.yaml Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
buf.gen.yaml Outdated Show resolved Hide resolved
src/Tracing/TracingServiceProvider.php Outdated Show resolved Hide resolved
src/lighthouse.php Outdated Show resolved Hide resolved
src/lighthouse.php Outdated Show resolved Hide resolved
src/lighthouse.php Outdated Show resolved Hide resolved
@cappuc
Copy link
Contributor Author

cappuc commented Dec 5, 2023

I've added a workflow to automatically update the reports.proto file from apollo. I set it to run weekly

CONTRIBUTING.md Outdated Show resolved Hide resolved
.github/workflows/update-reports-proto.yml Outdated Show resolved Hide resolved
buf.gen.yaml Show resolved Hide resolved
@spawnia spawnia added the enhancement A feature or improvement label Dec 5, 2023
@spawnia
Copy link
Collaborator

spawnia commented Dec 5, 2023

Apart from adding the changelog entry and updating the docs to say that federated tracing is supported, I am fine to go ahead and merge this. Excellent work!

Do you plan on updating the Apollo compatibility repository as well? I think we should just need to change tracing.driver in the configuration to make the example project there work?

@cappuc
Copy link
Contributor Author

cappuc commented Dec 5, 2023

We should take a look at error reporting (I wrote some notes in the first comment).

Apart from that and changelog/docs update it, everything should be fine.

Yes I think the only required change for apollo compatibility is the change of tracing.driver. I will make a PR to update apollo compatibility when this PR is merged.

@spawnia
Copy link
Collaborator

spawnia commented Dec 6, 2023

I am not sure how error reporting in tracing is/should be different from error reporting for clients. Does https://lighthouse-php.com/master/digging-deeper/error-handling.html apply? It would be nice to keep things simple and just utilize the same mechanisms. See \Nuwave\Lighthouse\GraphQL::errorsHandler().

@cappuc
Copy link
Contributor Author

cappuc commented Dec 6, 2023

Yes it apply all handlers.
I just changed the message to be "Internal server error" when error is not client safe and removed the json data, so we provide only message and location

@cappuc cappuc marked this pull request as ready for review December 6, 2023 14:15
docs/master/federation/getting-started.md Show resolved Hide resolved
docs/master/performance/tracing.md Outdated Show resolved Hide resolved
docs/master/performance/tracing.md Outdated Show resolved Hide resolved
src/Tracing/FederatedTracing/FederatedTracing.php Outdated Show resolved Hide resolved
src/Tracing/FederatedTracing/FederatedTracing.php Outdated Show resolved Hide resolved
src/Tracing/FederatedTracing/FederatedTracing.php Outdated Show resolved Hide resolved
@spawnia
Copy link
Collaborator

spawnia commented Dec 6, 2023

Ready to merge and release?

@cappuc
Copy link
Contributor Author

cappuc commented Dec 6, 2023

Yes, I think it's ready

@spawnia spawnia merged commit 3806fb6 into nuwave:master Dec 7, 2023
21 of 24 checks passed
@cappuc
Copy link
Contributor Author

cappuc commented Dec 7, 2023

Opened the PR to update the apollo compatibility

@cappuc cappuc deleted the feat/federated-tracing branch December 7, 2023 10:51
@Maxwell2022
Copy link

We need to update the doc too: https://lighthouse-php.com/6/federation/getting-started.html#federated-tracing

Experimental: not enabled by default, not guaranteed to be stable.

Is this still experimental?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Protocol buffer tracing standard Federated trace data
3 participants