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(TracerRegistry): set default name when nothing is provided #681

Closed
wants to merge 2 commits into from

Conversation

mayurkale22
Copy link
Member

Which problem is this PR solving?

/cc @dyladan @bg451

@mayurkale22 mayurkale22 added enhancement New feature or request size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 9, 2020
@codecov-io
Copy link

codecov-io commented Jan 9, 2020

Codecov Report

Merging #681 into master will decrease coverage by 1.82%.
The diff coverage is 98.3%.

@@            Coverage Diff             @@
##           master     #681      +/-   ##
==========================================
- Coverage   91.58%   89.75%   -1.83%     
==========================================
  Files         217      214       -3     
  Lines       10156    10077      -79     
  Branches      916      936      +20     
==========================================
- Hits         9301     9045     -256     
- Misses        855     1032     +177
Impacted Files Coverage Δ
...ry-tracing/test/export/ConsoleSpanExporter.test.ts 100% <ø> (ø) ⬆️
...ry-tracing/test/export/SimpleSpanProcessor.test.ts 100% <ø> (ø) ⬆️
...lemetry-plugin-http/test/functionals/utils.test.ts 99.35% <ø> (ø) ⬆️
packages/opentelemetry-web/test/WebTracer.test.ts 0% <ø> (ø) ⬆️
...entelemetry-exporter-zipkin/test/transform.test.ts 100% <ø> (ø) ⬆️
packages/opentelemetry-tracing/test/Span.test.ts 100% <ø> (ø) ⬆️
...try-tracing/test/export/BatchSpanProcessor.test.ts 100% <ø> (ø) ⬆️
...elemetry-plugin-dns/test/functionals/utils.test.ts 98.82% <ø> (ø) ⬆️
...opentelemetry-core/src/trace/globaltracer-utils.ts 81.81% <0%> (-18.19%) ⬇️
...ackages/opentelemetry-node/test/NodeTracer.test.ts 100% <100%> (ø) ⬆️
... and 55 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.

lgtm

packages/opentelemetry-tracing/src/BasicTracerRegistry.ts Outdated Show resolved Hide resolved
Copy link
Member

@OlivierAlbertini OlivierAlbertini left a comment

Choose a reason for hiding this comment

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

LGTM

@dyladan
Copy link
Member

dyladan commented Jan 13, 2020

See my comment on the related issue. I don't see a technical problem here, but I don't think we should merge something like this without addressing that.

@dyladan
Copy link
Member

dyladan commented Jan 14, 2020

Talked to spec about it today. The name property is definitely not optional. The reason the wording about having fallback behavior is in there is because some languages (javascript included) cannot strictly enforce that the api is called correctly.

@dyladan
Copy link
Member

dyladan commented Jan 14, 2020

There was also consensus in the spec today that the name will be used to namespace the metrics at least in some cases but it was not decided. I'm happy moving forward with this for now, but know that it will likely change in the near future.

@mayurkale22
Copy link
Member Author

@dyladan we should close this one, right?

@dyladan
Copy link
Member

dyladan commented Jan 16, 2020

Probably. The history will be cleaner if we make an update to the examples to give them good names on their own PR.

@dyladan dyladan closed this Jan 16, 2020
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
martinkuba pushed a commit to martinkuba/opentelemetry-js that referenced this pull request Mar 13, 2024
martinkuba pushed a commit to martinkuba/opentelemetry-js that referenced this pull request Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TracerRegistry: set name to default
6 participants