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

mayurkale22 commented Jan 9, 2020

Which problem is this PR solving?

/cc @dyladan @bg451

@codecov-io

This comment has been minimized.

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
@obecny
obecny approved these changes Jan 9, 2020
Copy link
Member

obecny left a comment

lgtm

Copy link
Member

OlivierAlbertini left a comment

LGTM

@dyladan

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member Author

mayurkale22 commented Jan 16, 2020

@dyladan we should close this one, right?

@dyladan

This comment has been minimized.

Copy link
Contributor

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.