-
Notifications
You must be signed in to change notification settings - Fork 842
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
Conversation
Codecov Report
@@ 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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. |
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. |
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. |
@dyladan we should close this one, right? |
Probably. The history will be cleaner if we make an update to the examples to give them good names on their own PR. |
Which problem is this PR solving?
default
#679/cc @dyladan @bg451