-
Notifications
You must be signed in to change notification settings - Fork 792
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
Initial skeleton #2
Initial skeleton #2
Conversation
I signed it |
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!
@draffensperger Can you please review this? |
package.json
Outdated
@@ -0,0 +1,32 @@ | |||
{ | |||
"name": "opentelemetry", |
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.
Should this be opentelemetry-base
similar to the @opencensus/opencensus-base
for OpenCensus?
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.
Done. Thanks
@flands Can we merge this? |
@rochdev Could you please review this? |
Will review by the end of the day. |
"description": "OpenTelemetry is a distributed tracing and stats collection framework.", | ||
"main": "build/src/index.js", | ||
"types": "build/src/index.d.ts", | ||
"scripts": { |
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.
There should be a script to do TDD in watch mode. I find this is usually very hard to add later on, especially in TypeScript projects that take an increasing amount of time to build.
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.
Sure, done. "tdd": "yarn run test -- --watch-extensions ts --watch"
"license": "Apache-2.0", | ||
"devDependencies": { | ||
"lerna": "^3.13.4", | ||
"typescript": "^3.4.5" |
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.
Was there ever a discussion to determine whether it makes sense to use TypeScript? As discussed in a few other issues already, I feel that for a Node library, and especially this type of project, it can actually make it very difficult to have for example different code paths per version of Node, or to test constructs that don't exist in every versions.
I think it would be a good exercise to actually weigh the pros and cons and not simply go with TypeScript because of its popularity. Also, do we have a plan for how watch mode would work with TypeScript?
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.
TypeScript was agreed upon by the majority. A guideline will be created to ensure it is used in a way that is compatible with this type of project.
package.json
Outdated
"types": "build/src/index.d.ts", | ||
"scripts": { | ||
"fix": "lerna run fix", | ||
"postinstall": "npm run bootstrap", |
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 was npm
used over yarn
? I try to go back to npm
pretty much every year but in the end yarn
is always superior.
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.
done
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. Left a few comments about a few things I think are worth discussing, but the structure itself looks good 👍
@rochdev Added tdd script in watch mode, removed lockfiles (as discussed in the meeting) and used yarn wherever needed. PTAL |
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. The TS compilation rules may need to be adjusted, but this can happen according to the TypeScript Guideline discussion after this is merged.
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, just a quick remark on the ts config
packages/tsconfig.base.json
Outdated
"allowUnusedLabels": false, | ||
"declaration": true, | ||
"forceConsistentCasingInFileNames": true, | ||
"lib": ["es2016"], |
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 don't think we need to bundle the es6 since we target es6,everything should be available at runtime
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. In general, I think a few rules will change once we work on the TypeScript guidelines if we decide to go with bundling basically nothing and relying on manually provided shims. (manually in the sense that we require them based on conditions, not that the user has to do it)
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.
Done.
d95fa67
to
539962a
Compare
chore: removing _cleanupGlobalShutdownListeners
Add ResourceProvider to sessions prototype
This PR contains,
npm init
lerna init
opentelemetry-core
package.tsconfig.base.json
in the root of our project, each individual package will have its owntsconfig.json
whose extended root.A high level package structure. The core package includes interfaces for trace, metrics, tags and resources along with no-op implementations.
New Update
API Package layout specs : open-telemetry/opentelemetry-specification#86