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

bump to @apollo/server v4 #574

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ Sometimes you don't want to trace everything, so we provide ways to select if yo
If you construct the extension with `shouldTraceRequest` you get the option to opt-in or out on a request basis.
When you don't start the span for the request the field resolvers will also not be used.

The function is called with the same arguments as the `requestDidStart` function extensions can provide, which is documented [here](https://github.com/apollographql/apollo-server/blob/master/packages/graphql-extensions/src/index.ts#L35).
The function is called with the same arguments as the `requestDidStart` function extensions can provide, which is documented [here](https://github.com/apollographql/apollo-server/blob/a1d549e/packages/server/src/externalTypes/plugins.ts#L52).

When the request is not traced there will also be no traces of the field resolvers.

Expand All @@ -101,7 +101,7 @@ There might be certain field resolvers that are not worth the tracing, e.g. when

## Modifying span metadata

If you'd like to add custom tags or logs to span you can construct the extension with `onRequestResolve`. The function is called with two arguments: span and infos `onRequestResolve?: (span: Span, info: RequestStart)`
If you'd like to add custom tags or logs to span you can construct the extension with `onRequestResolve`. The function is called with two arguments: span and infos `onRequestResolve?: (span: Span, info: GraphQLRequestContext)`
Copy link
Author

Choose a reason for hiding this comment

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

was contradicted by L117


## Using your own request span

Expand Down
1,635 changes: 786 additions & 849 deletions package-lock.json

Large diffs are not rendered by default.

13 changes: 4 additions & 9 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,26 +32,21 @@
"author": "Daniel Schmidt <[email protected]>",
"license": "MIT",
"peerDependencies": {
"apollo-server": ">=3.0.0",
"apollo-server-env": "*",
"@apollo/server": ">=4.0.0",
"graphql": ">=0.10.x",
"opentracing": "*"
},
"dependencies": {
"apollo-server-plugin-base": "^3.0.0",
"apollo-server-types": "^3.0.0"
},
"devDependencies": {
"@apollo/server": "4.5.0",
"@commitlint/config-conventional": "11.0.0",
"@commitlint/travis-cli": "11.0.0",
"@types/jest": "26.0.24",
"@types/node": "14.18.38",
"@types/supertest": "2.0.12",
"all-contributors-cli": "6.24.0",
"apollo-server": "3.12.0",
"apollo-server-env": "4.2.1",
"express": "4.18.2",
"graphql": "15.8.0",
"body-parser": "^1.20.2",
"graphql": "16.6.0",
"graphql-tools": "6.2.6",
"jest": "27.5.1",
"opentracing": "0.14.7",
Expand Down
18 changes: 9 additions & 9 deletions src/__tests__/__snapshots__/integration-test.ts.snap
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`integration with apollo-server alias with fragment works 1`] = `
exports[`integration with @apollo/server alias with fragment works 1`] = `
request:1
finished: true

Expand All @@ -13,7 +13,7 @@ request:1

`;

exports[`integration with apollo-server alias works 1`] = `
exports[`integration with @apollo/server alias works 1`] = `
request:1
finished: true

Expand All @@ -29,7 +29,7 @@ request:1

`;

exports[`integration with apollo-server correct span nesting 1`] = `
exports[`integration with @apollo/server correct span nesting 1`] = `
request:1
finished: true

Expand All @@ -45,7 +45,7 @@ request:1

`;

exports[`integration with apollo-server does not start a field resolver span if the parent field resolver was not traced 1`] = `
exports[`integration with @apollo/server does not start a field resolver span if the parent field resolver was not traced 1`] = `
request:1
finished: true

Expand All @@ -58,7 +58,7 @@ request:1

`;

exports[`integration with apollo-server implements traces for arrays 1`] = `
exports[`integration with @apollo/server implements traces for arrays 1`] = `
request:1
finished: true

Expand Down Expand Up @@ -86,7 +86,7 @@ request:1

`;

exports[`integration with apollo-server onFieldResolve & onFieldResolveFinish 1`] = `
exports[`integration with @apollo/server onFieldResolve & onFieldResolveFinish 1`] = `
request:1
finished: true

Expand Down Expand Up @@ -123,7 +123,7 @@ request:1

`;

exports[`integration with apollo-server onRequestError 1`] = `
exports[`integration with @apollo/server onRequestError 1`] = `
request:1
finished: true
logs:
Expand All @@ -135,7 +135,7 @@ request:1

`;

exports[`integration with apollo-server onRequestResolve 1`] = `
exports[`integration with @apollo/server onRequestResolve 1`] = `
request:1
finished: true
logs:
Expand All @@ -159,7 +159,7 @@ request:1

`;

exports[`integration with apollo-server picks up external spans 1`] = `
exports[`integration with @apollo/server picks up external spans 1`] = `
external:-1
finished: false

Expand Down
18 changes: 11 additions & 7 deletions src/__tests__/integration-test.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import * as express from "express";
import * as request from "supertest";
import { ApolloServer } from "apollo-server-express";
import { ApolloServer } from "@apollo/server";
import { expressMiddleware } from '@apollo/server/express4';
import { json } from 'body-parser';
import { Tracer } from "opentracing";
import { MockSpan, MockSpanTree } from "../test/types";
import spanSerializer from "../test/span-serializer";
import ApolloOpentracing, { InitOptions, SpanContext } from "../";
import ApolloOpentracing, { InitOptions } from "../";

expect.addSnapshotSerializer(spanSerializer);

Expand Down Expand Up @@ -147,16 +149,18 @@ class MockTracer {
}
}

async function createApp<InstanceContext extends SpanContext>({
interface EmptyContext {}

async function createApp({
tracer,
...params
}: { tracer: MockTracer } & Omit<
InitOptions<InstanceContext>,
Copy link
Author

Choose a reason for hiding this comment

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

Why was this passed as a generic?

InitOptions<EmptyContext>,
"server" | "local"
>) {
const app = express();

const server = new ApolloServer({
const server = new ApolloServer<EmptyContext>({
typeDefs: `
type A {
one: String
Expand Down Expand Up @@ -221,12 +225,12 @@ async function createApp<InstanceContext extends SpanContext>({
],
});
await server.start();
server.applyMiddleware({ app });
app.use('/graphql', json(), expressMiddleware(server));

return app;
}

describe("integration with apollo-server", () => {
describe("integration with @apollo/server", () => {
it("closes all spans", async () => {
const tracer = new MockTracer();
const app = await createApp({ tracer });
Expand Down
59 changes: 26 additions & 33 deletions src/context.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,16 @@
import { Span } from "opentracing";
import { GraphQLResolveInfo, ResponsePath } from "graphql";
import { GraphQLRequestContext } from "apollo-server-plugin-base";
import { ContextForApolloOpenTracing, WithRequiredSpanContext, hasSpanContext, addSpanContext } from './index';

/**
* Object containing private methods that this library uses, that will be attatched to the GQL context object.
* (Will be namespaced using a symbol to hide this from users)
*/
export interface SpanContext {
getSpanByPath(info: ResponsePath): Span | undefined;
addSpan(span: Span, info: GraphQLResolveInfo): void;
}

function isArrayPath(path: ResponsePath) {
return typeof path.key === "number";
}
Expand All @@ -19,40 +29,23 @@ export function buildPath(path: ResponsePath | undefined) {
return segments.reverse().join(".");
}

export interface SpanContext extends Object {
_spans: Map<string, Span>;
getSpanByPath(info: ResponsePath): Span | undefined;
addSpan(span: Span, info: GraphQLResolveInfo): void;
// Passed in from the outside context
requestSpan?: Span;
}

function isSpanContext(obj: any): obj is SpanContext {
return (
obj.getSpanByPath instanceof Function && obj.addSpan instanceof Function
);
}

export function requestIsInstanceContextRequest<CTX extends SpanContext>(
request: GraphQLRequestContext<CTX | Object>
): request is GraphQLRequestContext<CTX> {
return isSpanContext(request.context);
}

// TODO: think about using symbols to hide these
export function addContextHelpers(obj: any): SpanContext {
if (isSpanContext(obj)) {
return obj;
export function addContextHelpers<Context extends ContextForApolloOpenTracing>(
contextValue: Context
): WithRequiredSpanContext<Context> {
if (hasSpanContext(contextValue)) {
return contextValue;
}

obj._spans = new Map<string, Span>();
obj.getSpanByPath = function (path: ResponsePath): Span | undefined {
return this._spans.get(buildPath(isArrayPath(path) ? path.prev : path));
};

obj.addSpan = function (span: Span, info: GraphQLResolveInfo): void {
Comment on lines -49 to -53
Copy link
Author

Choose a reason for hiding this comment

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

Is context.getSpanByPath and context.addSpan supposed to be part of the public API provided by this library?

I don't see it referenced in README.md, other than: onFieldResolve(source: any, args: { [argName: string]: any }, context: SpanContext, info: GraphQLResolveInfo): Allow users to add extra information to the span which implies either:

  • you're supposed to be able to use these fields specifically when doing onFieldResolve, or;
  • this is just a consequence of mutating the context object and we're just being honest

(I've interpreted the latter given think about using symbols to hide these, but lmk!)

this._spans.set(buildPath(info.path), span);
const spans = new Map<string, Span>();
Copy link
Author

Choose a reason for hiding this comment

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

why was this attached to the context object? (closure here means it's no longer exposed)

const spanContext: SpanContext = {
getSpanByPath: (path: ResponsePath): Span | undefined => {
return spans.get(buildPath(isArrayPath(path) ? path.prev : path));
},
addSpan: (span: Span, info: GraphQLResolveInfo): void => {
spans.set(buildPath(info.path), span);
},
};

return obj;
addSpanContext(contextValue, spanContext);
return contextValue;
}
Loading