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.

Sign up for GitHub

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
merged 4 commits into from
Sep 24, 2019
Merged

fix: change options to config based on BasePlugin #314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
66f33d7
fix: change options to config based on BasePlugin
OlivierAlbertini Sep 22, 2019
a93f7a4
Merge branch 'master' into feature/options-to-config
OlivierAlbertini Sep 23, 2019
fc13220
Merge branch 'master' into feature/options-to-config
danielkhan Sep 24, 2019
66bfa00
Merge branch 'master' into feature/options-to-config
mayurkale22 Sep 24, 2019
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
2 changes: 1 addition & 1 deletion packages/opentelemetry-plugin-grpc/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
"node-pre-gyp": "^0.12.0",
"rimraf": "^3.0.0",
"tslint-microsoft-contrib": "^6.2.0",
"tslint-consistent-codestyle":"^1.15.1",
"tslint-consistent-codestyle": "^1.15.1",
"ts-mocha": "^6.0.0",
"ts-node": "^8.3.0",
"typescript": "^3.5.3"
Expand Down
6 changes: 2 additions & 4 deletions packages/opentelemetry-plugin-grpc/src/grpc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.


constructor(readonly moduleName: string, readonly version: string) {
super();
// TODO: remove this once options will be passed
// see https://github.com/open-telemetry/opentelemetry-js/issues/210
this.options = {};
this._config = {};
}

protected readonly _internalFilesList: ModuleExportsMapping = {
Expand Down
4 changes: 2 additions & 2 deletions packages/opentelemetry-plugin-grpc/test/grpc.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,10 +514,10 @@ describe('GrpcPlugin', () => {
});

before(() => {
plugin.enable(grpc, tracer, logger);
plugin.options = {
const config = {
// TODO: add plugin options here once supported
};
plugin.enable(grpc, tracer, logger, config);

const proto = grpc.load(PROTO_PATH).pkg_test;
server = startServer(grpc, proto);
Expand Down
18 changes: 8 additions & 10 deletions packages/opentelemetry-plugin-http/src/http.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,11 @@ import { Utils } from './utils';
*/
export class HttpPlugin extends BasePlugin<Http> {
static readonly component = 'http';
options!: HttpPluginConfig;
protected _config!: HttpPluginConfig;

constructor(readonly moduleName: string, readonly version: string) {
super();
// TODO: remove this once options will be passed
// see https://github.com/open-telemetry/opentelemetry-js/issues/210
this.options = {};
this._config = {};
}

/** Patches HTTP incoming and outcoming request functions. */
Expand Down Expand Up @@ -208,8 +206,8 @@ export class HttpPlugin extends BasePlugin<Http> {

span.setAttributes(attributes);

if (this.options.applyCustomAttributesOnSpan) {
this.options.applyCustomAttributesOnSpan(span, request, response);
if (this._config.applyCustomAttributesOnSpan) {
this._config.applyCustomAttributesOnSpan(span, request, response);
}

span.end();
Expand Down Expand Up @@ -248,7 +246,7 @@ export class HttpPlugin extends BasePlugin<Http> {

plugin._logger.debug('%s plugin incomingRequest', plugin.moduleName);

if (Utils.isIgnored(pathname, plugin.options.ignoreIncomingPaths)) {
if (Utils.isIgnored(pathname, plugin._config.ignoreIncomingPaths)) {
return original.apply(this, [event, ...args]);
}

Expand Down Expand Up @@ -314,8 +312,8 @@ export class HttpPlugin extends BasePlugin<Http> {
.setAttributes(attributes)
.setStatus(Utils.parseResponseStatus(response.statusCode));

if (plugin.options.applyCustomAttributesOnSpan) {
plugin.options.applyCustomAttributesOnSpan(span, request, response);
if (plugin._config.applyCustomAttributesOnSpan) {
plugin._config.applyCustomAttributesOnSpan(span, request, response);
}

span.end();
Expand Down Expand Up @@ -351,7 +349,7 @@ export class HttpPlugin extends BasePlugin<Http> {
options = optionsParsed;

if (
Utils.isIgnored(origin + pathname, plugin.options.ignoreOutgoingUrls)
Utils.isIgnored(origin + pathname, plugin._config.ignoreOutgoingUrls)
) {
return original.apply(this, [options, ...args]);
}
Expand Down
4 changes: 2 additions & 2 deletions packages/opentelemetry-plugin-http/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import { Span } from '@opentelemetry/types';
import { Span, PluginConfig } from '@opentelemetry/types';
import * as url from 'url';
import {
ClientRequest,
Expand Down Expand Up @@ -57,7 +57,7 @@ export interface HttpCustomAttributeFunction {
): void;
}

export interface HttpPluginConfig {
export interface HttpPluginConfig extends PluginConfig {
ignoreIncomingPaths?: IgnoreMatcher[];
ignoreOutgoingUrls?: IgnoreMatcher[];
applyCustomAttributesOnSpan?: HttpCustomAttributeFunction;
Expand Down
5 changes: 3 additions & 2 deletions packages/opentelemetry-plugin-http/test/functionals/http-enable.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
InMemorySpanExporter,
SimpleSpanProcessor,
} from '@opentelemetry/basic-tracer';
import { HttpPluginConfig } from '../../src';

let server: http.Server;
const serverPort = 22345;
Expand Down Expand Up @@ -80,17 +81,17 @@ describe('HttpPlugin', () => {
});

before(() => {
plugin.enable(http, tracer, tracer.logger);
const ignoreConfig = [
`http://${hostname}/ignored/string`,
/\/ignored\/regexp$/i,
(url: string) => url.endsWith(`/ignored/function`),
];
plugin.options = {
const config: HttpPluginConfig = {
ignoreIncomingPaths: ignoreConfig,
ignoreOutgoingUrls: ignoreConfig,
applyCustomAttributesOnSpan: customAttributeFunction,
};
plugin.enable(http, tracer, tracer.logger, config);

server = http.createServer((request, response) => {
response.end('Test Server Response');
Expand Down
11 changes: 6 additions & 5 deletions packages/opentelemetry-plugin-http/test/integrations/http-enable.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
InMemorySpanExporter,
SimpleSpanProcessor,
} from '@opentelemetry/basic-tracer';
import { HttpPluginConfig } from '../../src/types';

const serverPort = 32345;
const hostname = 'localhost';
Expand Down Expand Up @@ -68,20 +69,20 @@ describe('HttpPlugin Integration tests', () => {
});

before(() => {
try {
plugin.disable();
} catch (e) {}
plugin.enable(http, tracer, tracer.logger);
const ignoreConfig = [
`http://${hostname}:${serverPort}/ignored/string`,
/\/ignored\/regexp$/i,
(url: string) => url.endsWith(`/ignored/function`),
];
plugin.options = {
const config: HttpPluginConfig = {
ignoreIncomingPaths: ignoreConfig,
ignoreOutgoingUrls: ignoreConfig,
applyCustomAttributesOnSpan: customAttributeFunction,
};
try {
plugin.disable();
} catch (e) {}
plugin.enable(http, tracer, tracer.logger, config);
});

after(() => {
Expand Down