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

fix: change options to config based on BasePlugin #314

Merged

Conversation

OlivierAlbertini
Copy link
Member

Which problem is this PR solving?

Short description of the changes

  • Keep initialization in ctor in order to avoid undefined issue about _config property if we intend to use it outside PluginLoader. (related to Plugin config optional vs required #210)
  • use _config instead of options.

@OlivierAlbertini OlivierAlbertini force-pushed the feature/options-to-config branch from 4257164 to 66f33d7 Compare September 22, 2019 17:53
@codecov-io
Copy link

codecov-io commented Sep 22, 2019

Codecov Report

Merging #314 into master will decrease coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #314      +/-   ##
==========================================
- Coverage      98%   97.93%   -0.07%     
==========================================
  Files         101       98       -3     
  Lines        4912     4455     -457     
  Branches      412      384      -28     
==========================================
- Hits         4814     4363     -451     
+ Misses         98       92       -6
Impacted Files Coverage Δ
packages/opentelemetry-plugin-http/src/http.ts 96.34% <100%> (ø) ⬆️
...ckages/opentelemetry-plugin-grpc/test/grpc.test.ts 95.97% <100%> (ø) ⬆️
...-plugin-http/test/integrations/http-enable.test.ts 91.91% <100%> (ø) ⬆️
...y-plugin-http/test/functionals/http-enable.test.ts 98.17% <100%> (ø) ⬆️
packages/opentelemetry-plugin-grpc/src/grpc.ts 96.23% <100%> (ø) ⬆️
...ckages/opentelemetry-core/src/common/NoopLogger.ts 33.33% <0%> (-9.53%) ⬇️
.../opentelemetry-core/test/metrics/NoopMeter.test.ts 88.88% <0%> (-5.99%) ⬇️
...core/src/context/propagation/BinaryTraceContext.ts 97.67% <0%> (-0.69%) ⬇️
...entelemetry-core/test/common/ConsoleLogger.test.ts 100% <0%> (ø) ⬆️
...core/src/context/propagation/NoopHttpTextFormat.ts 100% <0%> (ø) ⬆️
... and 25 more

@@ -51,13 +51,11 @@ let grpcClientModule: object;
export class GrpcPlugin extends BasePlugin<grpc> {
static readonly component = 'grpc';

options!: GrpcPluginOptions;
protected _config!: GrpcPluginOptions;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why protected and not private?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why protected and not private?

It's protected on BasePlugin.

@mayurkale22 mayurkale22 merged commit 013a1e4 into open-telemetry:master Sep 24, 2019
@OlivierAlbertini OlivierAlbertini deleted the feature/options-to-config branch September 25, 2019 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plugins don't get config
7 participants