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(exporter-collector): default endpoint for node and browser #1197

Merged
merged 9 commits into from
Jun 17, 2020
Merged

fix(exporter-collector): default endpoint for node and browser #1197

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ac9f12f
Change default URL
davidwitten Jun 15, 2020
843eafe
Add tests
davidwitten Jun 15, 2020
f983d37
Fixed lint
davidwitten Jun 15, 2020
fa10cb3
Addressed comments
davidwitten Jun 15, 2020
fbcd430
Merge branch 'master' into defaulturl
davidwitten Jun 15, 2020
8878457
Set to get
davidwitten Jun 15, 2020
c4f73d6
Merge branch 'defaulturl' of https://github.com/davidwitten/opentelem…
davidwitten Jun 15, 2020
b1cc101
Added tests
davidwitten Jun 16, 2020
16978eb
Merge branch 'master' into defaulturl
mayurkale22 Jun 17, 2020
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-exporter-collector/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ const { CollectorExporter } = require('@opentelemetry/exporter-collector');

const collectorOptions = {
serviceName: 'basic-service',
url: '<opentelemetry-collector-url>' // url is optional and can be omitted - default is http://localhost:55678/v1/trace
url: '<opentelemetry-collector-url>' // url is optional and can be omitted - default is http://localhost:55678
Copy link
Member

Choose a reason for hiding this comment

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

I would mention both (Node and Web) default URLs.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mayurkale22 Should I also throw an error/alter the URL when someone enters an incorrect URL? (e.g. entering a URL that ends in /v1/trace for Node)

};

const provider = new BasicTracerProvider();
Expand Down
4 changes: 2 additions & 2 deletions packages/opentelemetry-exporter-collector/src/CollectorExporterBase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ export interface CollectorExporterConfigBase {
}

const DEFAULT_SERVICE_NAME = 'collector-exporter';
const DEFAULT_COLLECTOR_URL = 'http://localhost:55678/v1/trace';

/**
* Collector Exporter abstract base class
Expand All @@ -51,7 +50,7 @@ export abstract class CollectorExporterBase<
*/
constructor(config: T = {} as T) {
this.serviceName = config.serviceName || DEFAULT_SERVICE_NAME;
this.url = config.url || DEFAULT_COLLECTOR_URL;
this.url = this.defaultURL(config.url);
if (typeof config.hostName === 'string') {
this.hostName = config.hostName;
}
Expand Down Expand Up @@ -135,4 +134,5 @@ export abstract class CollectorExporterBase<
onSuccess: () => void,
onError: (error: CollectorExporterError) => void
): void;
abstract defaultURL(url: string | undefined): string;
}
6 changes: 6 additions & 0 deletions packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ import * as collectorTypes from '../../types';

export type CollectorExporterConfig = CollectorExporterConfigBase;

const DEFAULT_COLLECTOR_URL = 'http://localhost:55678/v1/trace';

/**
* Collector Exporter for Web
*/
Expand All @@ -38,6 +40,10 @@ export class CollectorExporter extends CollectorExporterBase<
window.removeEventListener('unload', this.shutdown);
}

defaultURL(url: string | undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

I would change the name to describe some action as defaultURL sounds like property and not a method. Maybe setDefaultUrl ?

Copy link
Member

Choose a reason for hiding this comment

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

For me, getDefaultUrl makes more sense than setDefaultUrl. We are not setting anything as a part of this method. WDYT @obecny @davidwitten ?

Copy link
Member

Choose a reason for hiding this comment

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

can be getDefaultUrl, chooseDefaultUrl etc. just something that sounds like action / method :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@obecny @mayurkale22 Just fixed that, thanks!

return url || DEFAULT_COLLECTOR_URL;
}

sendSpans(
spans: ReadableSpan[],
onSuccess: () => void,
Expand Down
6 changes: 6 additions & 0 deletions packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import { toCollectorExportTraceServiceRequest } from '../../transform';
import { GRPCQueueItem, TraceServiceClient } from './types';
import { removeProtocol } from './util';

const DEFAULT_COLLECTOR_URL = 'http://localhost:55678';
Copy link
Member

Choose a reason for hiding this comment

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

I think we should change this URL to localhost:55678 and remove removeProtocol logic from Node specific CollectorExporter module.

Copy link
Member

Choose a reason for hiding this comment

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

I would change the default url, but would live the removeProtocol to simply make it error proof for users


/**
* Collector Exporter Config for Node
*/
Expand Down Expand Up @@ -135,4 +137,8 @@ export class CollectorExporter extends CollectorExporterBase<
});
}
}

defaultURL(url: string | undefined): string {
return url || DEFAULT_COLLECTOR_URL;
}
}
3 changes: 3 additions & 0 deletions packages/opentelemetry-exporter-collector/test/common/CollectorExporter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ class CollectorExporter extends CollectorExporterBase<CollectorExporterConfig> {
onInit() {}
onShutdown() {}
sendSpans() {}
defaultURL(url: string | undefined) {
return url || '';
}
}

describe('CollectorExporter - common', () => {
Expand Down