usage reporting: add sendTraces, rename sendErrorsInTraces, etc #6855
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for apollo-server-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 0c2ffa7:
|
glasser
force-pushed
the
glasser/usage-reporting-tweaks
branch
from
6cd6f7c to
d155b36
Compare
- We've decided to leave the usage reporting defaults in AS4 as "field level instrumentation on for all operations, send some operations as traces". But we'd like to at least provide a non-experimental mechanism for entirely disabling sending traces to Studio. So there's a new ApolloServerPluginUsageReporting: `sendTraces: false`. - Rename the (new in AS4) usage reporting option `sendErrorsInTraces` to `sendErrors`, because it does also affect error statistics in stats reports if your `transform` function returns `null`. - Remove Apollo-internal `internal_includeTracesContributingToStats` option. This enabled some internal consistency monitoring which we are no longer paying attention to. - If you enable `debugPrintReports`, send the reports as `info` rather than `warn`, which primarily lets us delete some large comments (and makes sense because these debug messages are not warnings). Fixes #6051. Fixes #6078.
glasser
force-pushed
the
glasser/usage-reporting-tweaks
branch
from
d155b36 to
596b4f6
Compare
rkoron007
approved these changes
Aug 26, 2022
| * | ||
| * By setting the `sendTraces` option to `false`, Apollo Server will describe | ||
| * *all* operations as stats; individual requests will never be broken out | ||
| * into separate traces. If you set `sendTraces: false`, the Traces view in |
There was a problem hiding this comment.
How would you feel about rephrasing the second sentence to something like this:
Suggested change
| * into separate traces. If you set `sendTraces: false`, the Traces view in | |
| * into separate traces. If you set `sendTraces: false`, then Apollo Studio's Traces view won't show any traces ( other Studio functionality will be unaffected). |
| * Note that the values of `sendVariableValues`, `sendHeaders`, and | ||
| * `sendUnexecutableOperationDocuments` are irrelevant if you set | ||
| * `sendTraces: false`, because those options control data that is contained | ||
| * only in traces and not in stats. |
There was a problem hiding this comment.
I feel like parenthesis here help punctuate the point:
Suggested change
| * only in traces and not in stats. | |
| * only in traces (not in stats). |
| * be removed, and keys removed will be added back with an empty value. For security reasons, if an error occurs within this function, all variable values will be replaced with `[PREDICATE_FUNCTION_ERROR]`. | ||
| * - { exceptNames: ... }: a case-sensitive list of names of variables whose values should not be sent to Apollo servers | ||
| * - { onlyNames: ... }: A case-sensitive list of names of variables whose values will be sent to Apollo servers | ||
| * - { transform: ... }: a custom function for modifying variable values. Keys |
There was a problem hiding this comment.
I know this was in the original file, but the sentence: "Keys added by the custom function will be removed, and keys removed will be added back with an empty value" is confusing on the first read.
There was a problem hiding this comment.
@rkoron007 how's this?
* - { transform: ... }: a custom function for modifying variable values. The
* function receives `variables` and `operationString` and should return a
* record of `variables` with the same keys as the `variables` it receives
* (added variables will be ignored and removed variables will be reported
* with an empty value). For security reasons, if an error occurs within
* this function, all variable values will be replaced with
* `[PREDICATE_FUNCTION_ERROR]`.
There was a problem hiding this comment.
That is so clear, wonderful!
This was referenced Aug 26, 2022
glasser
pushed a commit
that referenced
this pull request
Aug 26, 2022
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to version-4, this PR will be updated.⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ `version-4` is currently in **pre mode** so this branch has prereleases rather than normal releases. If you want to exit prereleases, run `changeset pre exit` on `version-4`.⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ # Releases ## @apollo/[email protected] ### Patch Changes - [#6855](#6855) [`3e4ab3fca`](3e4ab3f) Thanks [@glasser](https://github.com/glasser)! - Rename usage reporting option `sendErrorsInTraces` (added in 4.0.0-alpha.4) to `sendErrors`, as it also affects error statistics outside of traces. - Updated dependencies \[[`3e4ab3fca`](3e4ab3f), [`3e4ab3fca`](3e4ab3f), [`3e4ab3fca`](3e4ab3f), [`3e4ab3fca`](3e4ab3f)]: - @apollo/[email protected] ## @apollo/[email protected] ### Patch Changes - [#6855](#6855) [`3e4ab3fca`](3e4ab3f) Thanks [@glasser](https://github.com/glasser)! - New usage reporting option `sendTraces: false` to only send usage reports as aggregated statistics, not per-request traces. - [#6855](#6855) [`3e4ab3fca`](3e4ab3f) Thanks [@glasser](https://github.com/glasser)! - Remove Apollo-internal `internal_includeTracesContributingToStats`. This should not have been used other than inside Apollo's own servers. - [#6855](#6855) [`3e4ab3fca`](3e4ab3f) Thanks [@glasser](https://github.com/glasser)! - The usage reporting option `debugPrintReports` now displays reports via `logger.info` rather than `logger.warn`. - [#6855](#6855) [`3e4ab3fca`](3e4ab3f) Thanks [@glasser](https://github.com/glasser)! - Rename usage reporting option `sendErrorsInTraces` (added in 4.0.0-alpha.4) to `sendErrors`, as it also affects error statistics outside of traces. Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Comment on lines
+32
to
+35
| * Note that the values of `sendVariableValues`, `sendHeaders`, and | ||
| * `sendUnexecutableOperationDocuments` are irrelevant if you set | ||
| * `sendTraces: false`, because those options control data that is contained | ||
| * only in traces (not in stats). |
There was a problem hiding this comment.
Maybe we should have sendTraces?: false | { sendVariableValues?, sendHeaders?, ... }
The nesting is a little extra, but there's no room for misconfiguration and the association of those options with traces should eliminate potential confusion.
There was a problem hiding this comment.
Oops, I should have acted on this feedback earlier.
What you're describing definitely feels like a superior API. Whether it's worth making it be yet another thing for users to mechanically update on upgrade (my intuition says sendVariableValues is a pretty popular option), not so sure.
| @@ -483,6 +483,8 @@ The function you pass to `transform` is called for each error (`GraphQLError`) t | |||
| - Return a modified form of the error (e.g., by changing the `err.message` to remove potentially sensitive information) | |||
| - Return `null` to prevent the error from being reported entirely | |||
|
|
|||
| Note that returning `null`` also affects Studio's aggregated statistics about how many operations contain errors and at what paths those errors appear. | |||
| // actually captured this as a full trace *and* | ||
| // sendOperationAsTrace says so. | ||
| // into stats) only if the user didn't set `sendTraces: false` | ||
| // *and8 we believe it's possible that our organization's plan |
There was a problem hiding this comment.
@bonnici caught this and fixed it elsewhere
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
level instrumentation on for all operations, send some operations as
traces". But we'd like to at least provide a non-experimental
mechanism for entirely disabling sending traces to Studio. So there's
a new ApolloServerPluginUsageReporting:
sendTraces: false.sendErrorsInTracestosendErrors, because it does also affect error statistics in statsreports if your
transformfunction returnsnull.internal_includeTracesContributingToStatsoption. This enabled some internal consistency monitoring which we are
no longer paying attention to.
debugPrintReports, send the reports asinforatherthan
warn, which primarily lets us delete some large comments (andmakes sense because these debug messages are not warnings).
Fixes #6051. Fixes #6078.