-
Notifications
You must be signed in to change notification settings - Fork 821
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
Api separation #727
Api separation #727
Conversation
Codecov Report
@@ Coverage Diff @@
## master #727 +/- ##
==========================================
- Coverage 92.47% 91.01% -1.46%
==========================================
Files 234 234
Lines 10475 10599 +124
Branches 944 959 +15
==========================================
- Hits 9687 9647 -40
- Misses 788 952 +164
|
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.
Overall I like the idea, added a few comments in the first phase.
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.
i didn't reviewed every files but it seems good, i mostly want to say that i agree with that changes.
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.
left few comments, lgtm, although not sure if we should change it this now or later for safety and also this change should be released with major version, not patch
} | ||
|
||
/** | ||
* Set the current global tracer. Returns the initialized global tracer registry |
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.
if this function only sets registry, it should have different name then init
Maybe add
or set
because it doesn't init anything
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.
This was brought over from the old core
package. A rename like this could be discussed though. cc @mayurkale22
import { Status } from './status'; | ||
import { TraceFlags } from './trace_flags'; | ||
|
||
export const INVALID_TRACE_ID = '0'; |
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.
why is it 0
and not empty for example ?
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.
This was the exact example in the old core
package. I think because it needs to satisfy the typings of the TraceContext
interface.
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
@markwolff and @xiao-lix it would be nice if you can review this change. |
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.
Overall SGTM. Agree with the concept as I think it is what other SIGs (e.g python) were already doing.
@@ -205,19 +200,19 @@ describe('Meter', () => { | |||
|
|||
it('should return no op metric if name is an empty string', () => { | |||
const counter = meter.createCounter(''); | |||
assert.ok(counter instanceof NoopMetric); | |||
assert.ok(counter instanceof types.NoopMetric); |
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.
nit: I usually expect * as types
imports to be only used for types and to be compiled away.
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.
An instanceof
cannot be compiled away because it is a runtime check. In any case, this is a test not deployed code.
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.
Yes sorry I should have been more clear. I am proposing renaming this to * as core
or * as api
, but as you've stated this is only a test so its only a nit :)
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.
Even if it wasn't a test, this is an instanceof
check which is a runtime check that can't be compiled away.
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.
I agree. My point is that because it is not being compiled away, using import * as types
here is misleading because it is being used for something other than just compile-time types.
Looks like one of our dependencies doesn't like node 8 anymore and requires 10 or higher |
Will have to update the documentation in |
* feat: create an api package * chore: update circle for new api package * chore: bring back getTracer * chore: add wrongly removed dev dependency * chore: review comments * chore: review comments * chore: lint * chore: export all noop implementations * chore: update API README * chore: ignore known working links that are not yet published * chore: add jsdoc for getInstance calls * chore: add jsdoc for private constructors * chore: review comments * chore: fix readme npm url * chore: fix old readmes without registry * chore: update api calling convention
* feat: create an api package * chore: update circle for new api package * chore: bring back getTracer * chore: add wrongly removed dev dependency * chore: review comments * chore: review comments * chore: lint * chore: export all noop implementations * chore: update API README * chore: ignore known working links that are not yet published * chore: add jsdoc for getInstance calls * chore: add jsdoc for private constructors * chore: review comments * chore: fix readme npm url * chore: fix old readmes without registry * chore: update api calling convention
Co-authored-by: Amir Blum <[email protected]> Co-authored-by: Bartlomiej Obecny <[email protected]>
Which problem is this PR solving?
Short description of the changes
@opentelemetry/types
to@opentelemetry/api
@opentelemetry/api
@opentelemetry/api
TODO:
browser tests for API