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

use the operation signature as graphql.document in spans #2703

Closed
wants to merge 10 commits into from

Conversation

Geal
Copy link
Contributor

@Geal Geal commented Mar 1, 2023

Fix #2695
Fix #2820

This will prevent personal information (in hardcoded variables) from appearing in logs. A few points to address here:

  • getting the the operation signature requires a successful query planning. What do we do if there was an error? We might need to know which query failed planning
  • the operation signature removes variables as expected, but also removes aliases, resulting in fields appearing multiple times, and reorders fields in some cases

Maybe here we could benefit from a router side solution instead of using the operation signature. Could we have a query transformation using apollo-compiler to remove indentation and hardcoded variables, but keep aliases and field order? @lrlna WDYT?

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

This will prevent personal information (in hardcoded variables) from
appearing in logs
@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2023

@Geal, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

@lrlna
Copy link
Member

lrlna commented Mar 2, 2023

Maybe here we could benefit from a router side solution instead of using the operation signature. Could we have a query transformation using apollo-compiler to remove indentation and hardcoded variables, but keep aliases and field order? @lrlna WDYT?

that'd be another good use case for modifying the original source in apollographql/apollo-rs#420

@Geal Geal requested review from BrynCooke and o0Ignition0o March 6, 2023 09:49
@Geal Geal marked this pull request as ready for review March 6, 2023 14:26
@BrynCooke
Copy link
Contributor

BrynCooke commented Mar 6, 2023

graphql.document is actually part of the spec:
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/instrumentation/graphql.md.
I understand why we want to limit this, but don't think we should deviate from the spec. Maybe just have an option to disable adding this to spans in the first place.

We could include the signature under a new graphql.signature key.

@Meemaw
Copy link
Contributor

Meemaw commented Mar 8, 2023

Having graphql.document as query is extremely useful imo. Could the router just replace inline/hardcoded variables with placeholders, e.g variable = $? That's what tracing tools usually do, for example for database queries which might include sensitive data.

@BrynCooke
Copy link
Contributor

We should also consider this option.

@Geal Geal requested a review from StephenBarlow as a code owner May 3, 2023 10:10
@Geal
Copy link
Contributor Author

Geal commented May 3, 2023

with 91a3497 there's now an option. Note that the operation signature is still added in all cases: https://github.com/apollographql/router/blob/dev/apollo-router/src/plugins/telemetry/mod.rs#L323-L331

@BrynCooke
Copy link
Contributor

I think some bits need updating, the changelog is incorrect and we should add some docs.

@abernix
Copy link
Member

abernix commented May 17, 2023

Is the signature we're referring to actually the "stats report key"?

@Geal
Copy link
Contributor Author

Geal commented May 17, 2023

Yes that's the one

@abernix
Copy link
Member

abernix commented May 19, 2023

We should not opaquely use the stats report key as the "graphql document" since it has a very Apollo-specific prefix on it. In the near term, if we want to use it to solve the stated goal, we should at the very least remove that (that is, the header part before and including the first \n).

@BrynCooke
Copy link
Contributor

@Geal Let's close this and tackle in #3226

@BrynCooke BrynCooke closed this Jun 23, 2023
@abernix abernix deleted the geal/use-operation-signature-in-spans branch May 3, 2024 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants