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

merged 9 commits into from
Jun 6, 2020

Conversation

rezakrimi
Copy link
Contributor

@rezakrimi rezakrimi commented Jun 3, 2020

Which problem is this PR solving?

Short description of the changes

  • Zipkin service name is set to resource 'service.name' if it's not specified in the config.

@codecov
Copy link

codecov bot commented Jun 3, 2020

Codecov Report

Merging #1138 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1138      +/-   ##
==========================================
+ Coverage   92.31%   92.32%   +0.01%     
==========================================
  Files         116      116              
  Lines        3397     3402       +5     
  Branches      685      689       +4     
==========================================
+ Hits         3136     3141       +5     
  Misses        261      261              
Impacted Files Coverage Δ
...ackages/opentelemetry-exporter-zipkin/src/types.ts 100.00% <ø> (ø)
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts 98.46% <100.00%> (+0.12%) ⬆️

making service name optional for zipkin exporter

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
making config object optional for zipkin

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Copy link
Member

@mayurkale22 mayurkale22 left a comment

Choose a reason for hiding this comment

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

Overall LGTM, added two comments.

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

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.

if (typeof this._serviceName !== 'string') {
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

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

@mayurkale22 mayurkale22 added the enhancement New feature or request label Jun 5, 2020
@linux-foundation-easycla
Copy link

CLA Check

  • rezakrimi The commit (6c113d7 ,e0dd7425bad29e48d51c6c4f120c9dd9909e355d ,1ce71b9b6a2c1be0baff6fc4ccf921a66f0115ec ,c0b56d37025dcfa9cb4caae7a54684c80661b657 ,42de702f41931584b0f551c4cbabd4b30152b1c0 ,ad18c2146285f1ed43c8e758459fc8c62e4d2ddd ,c7cf1181e97c9eb62863796277c414842a8b6032) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

@mayurkale22 mayurkale22 merged commit a4da9de into open-telemetry:master Jun 6, 2020
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
…ing open-telemetry#1136 (open-telemetry#1138)

* ci: [Bug] DNS Lookup and Promise Lookup test for error NOT_FOUND failing open-telemetry#1136

* chore: automatically fix lint issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use resource service.name as zipkin service name
4 participants