Skip to content

Conversation

@obecny
Copy link
Member

@obecny obecny commented Dec 8, 2020

Which problem is this PR solving?

Short description of the changes

  • Adds auto loader for instrumentation and old plugins
  • Copy the plugin loader to instrumentation including this change -> Feat/plugin injection #1704
  • It is possible to define something like this
  1. Node
const httpPlugin = require('@opentelemetry/plugin-http').plugin;
const { GraphQLInstrumentation } = require('@opentelemetry/instrumentation-graphql');

registerInstrumentations({
  instrumentations: [
    GraphQLInstrumentation,
    {
      plugins: {
        http: { enabled: true, plugin: httpPlugin },
        https: { enabled: false, path: '@opentelemetry/plugin-https' },
        express: { enabled: true, path: `${currentDir}/node_modules/@opentelemetry/plugin-express/build/src/` },
      },
    },
  ],
  tracerProvider: provider,
  logger: new ConsoleLogger()
});

  1. Web
import { registerInstrumentations } from '@opentelemetry/instrumentation';
import { UserInteractionPlugin } from '@opentelemetry/plugin-user-interaction';
import { DocumentLoad } from '@opentelemetry/plugin-document-load';
import { XMLHttpRequestInstrumentation } from '@opentelemetry/instrumentation-xml-http-request';

registerInstrumentations({
  instrumentations: [
    new XMLHttpRequestInstrumentation({
      ignoreUrls: [/localhost:8090\/sockjs-node/],
      propagateTraceHeaderCorsUrls: [
        'https://httpbin.org/get',
      ],
    }),
    new UserInteractionPlugin(),
    new DocumentLoad(),
  ],
  tracerProvider: providerWithZone,
});

So mixing old plugins and new instrumentation is possible now

Update:
test added

@obecny
Copy link
Member Author

obecny commented Dec 8, 2020

@open-telemetry/javascript-approvers have a look before I start moving tests and create new one

@codecov
Copy link

codecov bot commented Dec 8, 2020

Codecov Report

Merging #1731 (83ebaa3) into master (b4c9e40) will increase coverage by 0.29%.
The diff coverage is 95.97%.

@@            Coverage Diff             @@
##           master    #1731      +/-   ##
==========================================
+ Coverage   92.18%   92.47%   +0.29%     
==========================================
  Files         167      173       +6     
  Lines        5845     6019     +174     
  Branches     1256     1287      +31     
==========================================
+ Hits         5388     5566     +178     
+ Misses        457      453       -4     
Impacted Files Coverage Δ
...try-instrumentation/src/platform/node/old/utils.ts 90.47% <90.47%> (ø)
...trumentation/src/platform/node/old/PluginLoader.ts 93.97% <93.97%> (ø)
...es/opentelemetry-instrumentation/src/autoLoader.ts 100.00% <100.00%> (ø)
...entelemetry-instrumentation/src/autoLoaderUtils.ts 100.00% <100.00%> (ø)
...rumentation/src/platform/browser/old/autoLoader.ts 100.00% <100.00%> (ø)
...nstrumentation/src/platform/node/old/autoLoader.ts 100.00% <100.00%> (ø)
... and 3 more

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Seems fine to me except the one weird type assertion thing

@obecny obecny self-assigned this Dec 17, 2020
@obecny obecny marked this pull request as ready for review December 18, 2020 16:57
@obecny
Copy link
Member Author

obecny commented Dec 18, 2020

@dyladan dyladan added enhancement New feature or request Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) labels Dec 22, 2020
@dyladan
Copy link
Member

dyladan commented Jan 7, 2021

@obecny looks like everyone who is going to review has done so. if you're ready i'm ok if you merge this

@obecny obecny merged commit 89664a7 into open-telemetry:master Jan 7, 2021
@obecny obecny deleted the autoloader branch January 7, 2021 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Instrumentation Autoload

3 participants