Graphql plugin - temporary here for reviews - will be moved to contrib repo later #1573
Conversation
obecny
requested review from
dyladan,
legendecas,
markwolff,
mayurkale22,
mwear,
naseemkullah,
OlivierAlbertini and
vmarchaud
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1573 +/- ##
=======================================
Coverage 92.93% 92.93%
=======================================
Files 156 156
Lines 4871 4871
Branches 988 988
=======================================
Hits 4527 4527
Misses 344 344 |
obecny
changed the title
obecny
changed the title
|
@open-telemetry/javascript-approvers ^^ |
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| // is moving to instrumentation |
There was a problem hiding this comment.
#1572 has been merged so this need to be updated
| private _addPatchingExecute(): InstrumentationNodeModuleFile< | ||
| typeof graphqlTypes | ||
| > { | ||
| const file = new InstrumentationNodeModuleFile<typeof graphqlTypes>( |
There was a problem hiding this comment.
could we just return the newly created class instead of assigning it ?
| 'graphql/execution/execute.js', | ||
| // cannot make it work with appropriate type as execute function has 2 | ||
| //types and/cannot import function but only types | ||
| (moduleExports: any) => { |
There was a problem hiding this comment.
can we use unknown here and check manuallly that the wrapped functions exist ?
| document: graphqlTypes.DocumentNode, | ||
| rootValue: any, | ||
| contextValue: any, | ||
| variableValues: Maybe<{ [key: string]: any }>, |
There was a problem hiding this comment.
Could be Maybe<Record<string, any>>
There was a problem hiding this comment.
it needs to be the same as in graphql
| */ | ||
|
|
||
| export * from './graphql'; | ||
| export * from './symbols'; |
There was a problem hiding this comment.
why should we export the symbols ? If thats just for the tests, could we just directly import them ?
| * The maximum depth of fields/resolvers to instrument. | ||
| * When set to 0 it will not instrument fields and resolvers | ||
| * | ||
| * @default undefined |
|
closing in favour of open-telemetry/opentelemetry-js-contrib#226 |
|
@vmarchaud deleted my comment but it seems I have deleted yours accidently :/ |
I'm pushing this here as instrumentation has not been published yet, once it is done I'm happy to move it to contrib, but for easier review this can be reviewed here too.