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

feat(opentelemetry-js): infer zipkin service name from resource #1138

Merged
merged 9 commits into from
Jun 6, 2020
Merged

feat(opentelemetry-js): infer zipkin service name from resource #1138

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6c113d7
feat(opentelemetry-js): infer zipkin service name from resource
rezakrimi Jun 3, 2020
e0dd742
Update packages/opentelemetry-exporter-zipkin/src/zipkin.ts
rezakrimi Jun 3, 2020
1ce71b9
fix: making serviceName optional for zipkin config
rezakrimi Jun 3, 2020
c0b56d3
fix: resolving type issues
rezakrimi Jun 3, 2020
42de702
Update packages/opentelemetry-exporter-zipkin/src/zipkin.ts
rezakrimi Jun 3, 2020
ad18c21
refactor: making default service name a constant
rezakrimi Jun 4, 2020
c7cf118
Merge branch 'zipkin-service-name' of https://github.com/rezakrimi/op…
rezakrimi Jun 4, 2020
edcc5f9
Merge branch 'master' into zipkin-service-name
mayurkale22 Jun 5, 2020
bfc8566
Merge branch 'master' into zipkin-service-name
mayurkale22 Jun 6, 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-zipkin/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import * as api from '@opentelemetry/api';
*/
export interface ExporterConfig {
logger?: api.Logger;
serviceName: string;
serviceName?: string;
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if you can update the https://github.com/rezakrimi/opentelemetry-js/tree/zipkin-service-name/packages/opentelemetry-exporter-zipkin#usage and mention "serviceName is optional and can be omitted - it will be inferred from resource and in case not provided, serviceName will default to "OpenTelemetry Service"". Something like this.

Copy link
Contributor Author

@rezakrimi rezakrimi Jun 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

How about we mention that it's completely optional to pass the config now. So the usage section will be something like:
"Install the exporter on your application and pass the options. It's optional to pass the options. If serviceName is omitted, it will be inferred from resource and in case not provided, serviceName will default to "OpenTelemetry Service". The url is also set to http://localhost:9411/api/v2/spans by default."

Copy link
Member

Choose a reason for hiding this comment

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

It's not exactly obvious how to set the service name in the resource right now, so I would focus more on the case where you do directly set the service name:

serviceName is an optional string. If omitted, the exporter will first try to get the service name from the Resource. If no service name can be detected on the Resource, a fallback name of "OpenTelemetry Service" will be used.

url?: string;
// Optional mapping overrides for OpenTelemetry status code and description.
statusCodeTagName?: string;
Expand Down
15 changes: 5 additions & 10 deletions packages/opentelemetry-exporter-zipkin/src/zipkin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,15 +70,10 @@ export class ZipkinExporter implements SpanExporter {
resultCallback: (result: ExportResult) => void
) {
if (typeof this._serviceName !== 'string') {
dyladan marked this conversation as resolved.
Show resolved Hide resolved
if (
typeof spans[0].resource.labels[SERVICE_RESOURCE.NAME] !== 'undefined'
) {
this._serviceName = spans[0].resource.labels[
SERVICE_RESOURCE.NAME
].toString();
} else {
this._serviceName = 'Unnamed Service';
}
this._serviceName = String(
spans[0].resource.labels[SERVICE_RESOURCE.NAME] ||
'OpenTelemetry Service'
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 prefer to have const for this.

Copy link
Contributor Author

@rezakrimi rezakrimi Jun 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

do you mean like thi?

        const defaultServiceName = String(
        spans[0].resource.labels[SERVICE_RESOURCE.NAME] ||
          'OpenTelemetry Service');

Copy link
Member

Choose a reason for hiding this comment

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

No he means defining a constant somewhere like const DEFAULT_SERVICE_NAME = 'OpenTelemetry Service' then doing

String(spans[0].resource.labels[SERVICE_RESOURCE.NAME] || DEFAULT_SERVICE_NAME);

);
}
this._logger.debug('Zipkin exporter export');
if (this._isShutdown) {
Expand All @@ -105,7 +100,7 @@ export class ZipkinExporter implements SpanExporter {
private _toZipkinSpan(span: ReadableSpan): zipkinTypes.Span {
return toZipkinSpan(
span,
this._serviceName,
String(this._serviceName),
dyladan marked this conversation as resolved.
Show resolved Hide resolved
this._statusCodeTagName,
this._statusDescriptionTagName
);
Expand Down
16 changes: 4 additions & 12 deletions packages/opentelemetry-exporter-zipkin/test/zipkin.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ describe('ZipkinExporter', () => {
});
});

it('should set serviceName to "Unnamed service" by default', () => {
it('should set serviceName to "Opentelemetry Service" by default', () => {
const scope = nock('http://localhost:9411')
.post('/api/v2/spans')
.replyWithError(new Error('My Socket Error'));
Expand Down Expand Up @@ -393,15 +393,11 @@ describe('ZipkinExporter', () => {
resource: Resource.empty(),
};

const exporter = new ZipkinExporter({
serviceName: 'my-service',
});

delete exporter['_serviceName'];
const exporter = new ZipkinExporter({});

exporter.export([span1, span2], (result: ExportResult) => {
scope.done();
assert.equal(exporter['_serviceName'], 'Unnamed Service');
assert.equal(exporter['_serviceName'], 'OpenTelemetry Service');
});
});

Expand Down Expand Up @@ -469,11 +465,7 @@ describe('ZipkinExporter', () => {
resource: Resource.empty(),
};

const exporter = new ZipkinExporter({
serviceName: 'my-service',
});

delete exporter['_serviceName'];
const exporter = new ZipkinExporter({});

exporter.export([span1, span2], (result: ExportResult) => {
scope.done();
Expand Down