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

feat: creating one auto loader for instrumentation and old plugins #1731

Merged
merged 13 commits into from
Jan 7, 2021

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
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

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