Skip to content
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
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: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ The version headers in this history reflect the versions of Apollo Server itself
- [__CHANGELOG for `@apollo/gateway`__](https://github.com/apollographql/apollo-server/blob/master/packages/apollo-gateway/CHANGELOG.md)
- [__CHANGELOG for `@apollo/federation`__](https://github.com/apollographql/apollo-server/blob/master/packages/apollo-federation/CHANGELOG.md)

### vNEXT

- `apollo-engine-reporting`: Deprecated the `APOLLO_SCHEMA_TAG` environment variable in favor of its new name, `APOLLO_GRAPH_VARIANT`. Similarly, within the `engine` configuration object, the `schemaTag` property has been renamed `graphVariant`. The functionality remains otherwise unchanged, but their new names mirror the name used within Apollo Graph Manager. Continued use of the now-deprecated names will result in deprecation warnings and support will be dropped completely in the next "major" update. To avoid misconfiguration, a runtime error will be thrown if _both_ new and deprecated names are set. [PR #3855](https://github.com/apollographql/apollo-server/pull/3855)

### v2.11.0

- The range of accepted `peerDepedencies` versions for `graphql` has been widened to include `graphql@^15.0.0-rc.2` so as to accommodate the latest release-candidate of the `graphql@15` package, and an intention to support it when it is finally released on the `latest` npm tag. While this change will subdue peer dependency warnings for Apollo Server packages, many dependencies from outside of this repository will continue to raise similar warnings until those packages own `peerDependencies` are updated. It is unlikely that all of those packages will update their ranges prior to the final version of `graphql@15` being released, but if everything is working as expected, the warnings can be safely ignored. [PR #3825](https://github.com/apollographql/apollo-server/pull/3825)
Expand Down
2 changes: 1 addition & 1 deletion docs/source/federation/metrics.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ Ensure that all dependencies on `apollo-server` are at version `2.7.0` or higher

These options will cause the Apollo gateway to collect tracing information from the underlying federated services and pass them on, along with the query plan, to the Apollo metrics ingress. Currently, only Apollo Server supports detailed metrics insights as an implementing service, but we would love to work with you to implement the protocol in other languages!

> NOTE: By default, metrics will be reported to the `current` variant. To change the variant for reporting, set the `ENGINE_GRAPH_VARIANT` environment variable.
> NOTE: By default, metrics will be reported to the `current` variant. To change the variant for reporting, set the `APOLLO_GRAPH_VARIANT` environment variable.

## How tracing data is exposed from a federated service

Expand Down
32 changes: 26 additions & 6 deletions packages/apollo-engine-reporting/src/agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,29 @@ export type GenerateClientInfo<TContext> = (
requestContext: GraphQLRequestContext<TContext>,
) => ClientInfo;

// AS3: Drop support for deprecated bits.
export function getEngineGraphVariant(engine: EngineReportingOptions<any> | boolean | undefined): string | undefined {
Comment thread
zionts marked this conversation as resolved.
Outdated
Comment thread
abernix marked this conversation as resolved.
Outdated
if (engine === false) {
return;
} else if (typeof engine === 'object' && (engine.graphVariant || engine.schemaTag)) {
if (engine.graphVariant && engine.schemaTag) {
throw new Error('Cannot set both engine.graphVariant and engine.schemaTag. Please use engine.graphVariant.');
}
if (engine.schemaTag) {
console.warn('[Deprecation warning] Usage of engine.schemaTag is deprecated. Please use engine.graphVariant instead.');
Comment thread
zionts marked this conversation as resolved.
Outdated
}
return engine.graphVariant || engine.schemaTag;
} else {
if (process.env.ENGINE_SCHEMA_TAG) {
console.warn('[Deprecation warning] Usage of ENGINE_SCHEMA_TAG is deprecated. Please use APOLLO_GRAPH_VARIANT instead.');
}
if (process.env.ENGINE_SCHEMA_TAG && process.env.APOLLO_GRAPH_VARIANT) {
throw new Error('Cannot set both ENGINE_SCHEMA_TAG and APOLLO_GRAPH_VARIANT. Please use APOLLO_GRAPH_VARIANT.')
}
return process.env.APOLLO_GRAPH_VARIANT || process.env.ENGINE_SCHEMA_TAG;
}
}

export interface EngineReportingOptions<TContext> {
/**
* API key for the service. Get this from
Expand Down Expand Up @@ -214,6 +237,7 @@ const serviceHeaderDefaults = {
export class EngineReportingAgent<TContext = any> {
private options: EngineReportingOptions<TContext>;
private apiKey: string;
private graphVariant: string;
private reports: { [schemaHash: string]: FullTracesReport } = Object.create(
null,
);
Expand All @@ -231,6 +255,7 @@ export class EngineReportingAgent<TContext = any> {
public constructor(options: EngineReportingOptions<TContext> = {}) {
this.options = options;
this.apiKey = options.apiKey || process.env.ENGINE_API_KEY || '';
this.graphVariant = getEngineGraphVariant(options) || '';
if (!this.apiKey) {
throw new Error(
'To use EngineReportingAgent, you must specify an API key via the apiKey option or the ENGINE_API_KEY environment variable.',
Expand Down Expand Up @@ -294,12 +319,7 @@ export class EngineReportingAgent<TContext = any> {
this.reportHeaders[schemaHash] = new ReportHeader({
...serviceHeaderDefaults,
schemaHash,
schemaTag:
this.options.graphVariant
|| this.options.schemaTag
|| process.env.APOLLO_GRAPH_VARIANT
|| process.env.ENGINE_SCHEMA_TAG
|| '',
schemaTag: this.graphVariant,
});
// initializes this.reports[reportHash]
this.resetReport(schemaHash);
Expand Down
1 change: 1 addition & 0 deletions packages/apollo-gateway/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
## vNEXT

- Fix Typescript generic typing for datasource contexts [#3865](https://github.com/apollographql/apollo-server/pull/3865) This is a fix for the `TContext` typings of the gateway's exposed `GraphQLDataSource` implementations. In their current form, they don't work as intended, or in any manner that's useful for typing the `context` property throughout the class methods. This introduces a type argument `TContext` to the class itself (which defaults to `Record<string, any>` for existing implementations) and removes the non-operational type arguments on the class methods themselves.
- Deprecated the `APOLLO_SCHEMA_TAG` environment variable in favor of its new name, `APOLLO_GRAPH_VARIANT`. The functionality remains otherwise identical, but the new name mirrors the name used within Apollo Graph Manager. Use of the now-deprecated name will result in a deprecation warning and support will be dropped completely in a future "major" update. To avoid misconfiguration, runtime errors will be thrown if the new and deprecated name are _both_ set. [#3855](https://github.com/apollographql/apollo-server/pull/3855)

## 0.13.2

Expand Down
17 changes: 1 addition & 16 deletions packages/apollo-server-core/src/ApolloServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ import {

import { Headers } from 'apollo-server-env';
import { buildServiceDefinition } from '@apollographql/apollo-tools';
import {getEngineGraphVariant} from "apollo-engine-reporting/dist/agent";

const NoIntrospection = (context: ValidationContext) => ({
Field(node: FieldDefinitionNode) {
Expand All @@ -94,22 +95,6 @@ function getEngineApiKey(engine: Config['engine']): string | undefined {
return;
}

function getEngineGraphVariant(engine: Config['engine']): string | undefined {
if (engine === false) {
return;
} else if (typeof engine === 'object' && (engine.graphVariant || engine.schemaTag)) {
return engine.graphVariant || engine.schemaTag;
} else {
if (process.env.ENGINE_SCHEMA_TAG) {
console.warn('[Deprecation warning] Usage of ENGINE_SCHEMA_TAG is deprecated. Please use APOLLO_GRAPH_VARIANT instead.');
}
if (process.env.ENGINE_SCHEMA_TAG && process.env.APOLLO_GRAPH_VARIANT) {
throw new Error('Cannot set both ENGINE_SCHEMA_TAG and APOLLO_GRAPH_VARIANT. Please use APOLLO_GRAPH_VARIANT.')
}
return process.env.APOLLO_GRAPH_VARIANT || process.env.ENGINE_SCHEMA_TAG;
}
}

function getEngineServiceId(engine: Config['engine']): string | undefined {
const engineApiKey = getEngineApiKey(engine);
if (engineApiKey) {
Expand Down
44 changes: 44 additions & 0 deletions packages/apollo-server-core/src/__tests__/ApolloServerBase.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,50 @@ describe('ApolloServerBase construction', () => {
).not.toThrow();
});

it('succeeds when passed a graphVariant in construction', () => {
let serverBase;
expect(
() =>
new ApolloServerBase({
schema: buildServiceDefinition([{ typeDefs, resolvers }]).schema,
Comment thread
zionts marked this conversation as resolved.
Outdated
Comment thread
zionts marked this conversation as resolved.
Outdated
engine: {
graphVariant: 'foo',
apiKey: 'not:real:key',
},
}).stop()
).not.toThrow();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we could be more specific here and test for what we don't want thrown, in terms of specific error message. Otherwise this test ends up failing when some other thing which intentionally fails this pattern crops up, red herring during development, etc. We can avoid that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

On the flip side, it means that if someone changes the error message, then the test which is looking for it will continue to pass silently, but will be doing nothing, right?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@zionts A defensive best-of-both-worlds technique would be test for each and have the tests rely on the same error message string.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But like... what error message are we looking for? I can think of a couple different error messages that are reasonable to test for.

  • defined both graphVariant and schemaTag
  • unknown key / TypeError

Will we actually keep this up to date as we add more? Will it actually be a red herring as opposed to a good test update, when we add, e.g. "throw on malformed API key" that causes this test to error?

I would really prefer to leave this as just making sure this configuration doesn't throw anything and updating the test + adding another if another behavior through a different feature causes this test to throw.

});
Comment on lines +33 to +46
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I really feel this test belongs in apollo-engine-reporting since engine is implemented by that package. (I think we should continue to believe that AER is a separate package until it is not.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm down for adding another test in apollo-engine-reporting, but my main goal was to test what I care about: that Apollo Server builds.

Comment thread
zionts marked this conversation as resolved.

it('spits out a deprecation warning when passed a schemaTag in construction', () => {
const spyConsoleWarn = jest.spyOn(console, 'warn').mockImplementation();
expect(
() =>
new ApolloServerBase({
schema: buildServiceDefinition([{ typeDefs, resolvers }]).schema,
Comment thread
zionts marked this conversation as resolved.
Outdated
engine: {
schemaTag: 'foo',
apiKey: 'not:real:key',
},
}).stop()
).not.toThrow();
expect(spyConsoleWarn).toBeCalled();
spyConsoleWarn.mockRestore();
});

it('throws when passed a schemaTag and graphVariant in construction', () => {
expect(
() =>
new ApolloServerBase({
schema: buildServiceDefinition([{ typeDefs, resolvers }]).schema,
engine: {
schemaTag: 'foo',
graphVariant: 'heck',
apiKey: 'not:real:key',
},
}),
).toThrow();
});

it('throws when a GraphQLSchema is not provided to the schema configuration option', () => {
expect(() => {
new ApolloServerBase({
Expand Down