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(exporter-zipkin): per-span service name #1789

Merged

Conversation

sfishel-splunk
Copy link
Contributor

…wn attributes or its resource's attributes

Which problem is this PR solving?

  • My use case is instrumenting a frontend single-page-application where I'd like to model different parts of the app as different services from a tracing perspective. But I want all parts of the application to share a single instance of the Zipkin exporter.

Short description of the changes

  • When converting the spans to the Zipkin format, check for a service name defined either in the span's attributes or in its resource's attributes, and allow that service name to override the one provided to the exporter constructor.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 5, 2021

CLA Signed

The committers are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Jan 5, 2021

Codecov Report

Merging #1789 (f96fec0) into master (fd0d35d) will decrease coverage by 0.23%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1789      +/-   ##
==========================================
- Coverage   92.60%   92.36%   -0.24%     
==========================================
  Files         174      157      -17     
  Lines        6042     5108     -934     
  Branches     1283     1091     -192     
==========================================
- Hits         5595     4718     -877     
+ Misses        447      390      -57     
Impacted Files Coverage Δ
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts 100.00% <100.00%> (ø)
...ages/opentelemetry-exporter-collector/src/types.ts
...kages/opentelemetry-web/src/StackContextManager.ts
...s/opentelemetry-instrumentation-fetch/src/fetch.ts
.../opentelemetry-exporter-collector/src/transform.ts
packages/opentelemetry-web/src/types.ts
...ry-context-zone-peer-dep/src/ZoneContextManager.ts
...mentation-xml-http-request/src/enums/EventNames.ts
...ntelemetry-web/src/enums/PerformanceTimingNames.ts
...ackages/opentelemetry-web/src/WebTracerProvider.ts
... and 8 more

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

few comments, please add unit test for the changes

@dyladan dyladan added the enhancement New feature or request label Jan 13, 2021
@dyladan dyladan changed the title fix(exporter-zipkin): support per-span service name either from its o… feat(exporter-zipkin): support per-span service name Jan 13, 2021
@dyladan dyladan changed the title feat(exporter-zipkin): support per-span service name feat(exporter-zipkin): per-span service name Jan 13, 2021
@vmarchaud
Copy link
Member

Thanks for the contribution @sfishel-splunk !

@vmarchaud vmarchaud merged commit 471306f into open-telemetry:master Jan 15, 2021
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.

None yet

4 participants