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

Initial skeleton #2

Merged
merged 6 commits into from Jun 10, 2019

Conversation

Projects
None yet
6 participants
@mayurkale22
Copy link
Contributor

commented May 15, 2019

This PR contains,

  1. npm init
  2. lerna init
  3. Created a opentelemetry-core package.
  4. Added common tsconfig.base.json in the root of our project, each individual package will have its own tsconfig.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.

opentelemetry/core
   |__ trace/
      |__ propagation/
      |__ samplers/
   |__ metrics/
      |__ stats/
   |__ tags/
      |__ propagation/
   |__ resources/
   |__ utils/

New Update

API Package layout specs : open-telemetry/opentelemetry-specification#86

@mayurkale22 mayurkale22 requested review from flands and justindsmith May 15, 2019

@mayurkale22

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

I signed it

@justindsmith
Copy link

left a comment

LGTM!

@mayurkale22

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

@draffensperger Can you please review this?

@@ -0,0 +1,32 @@
{
"name": "opentelemetry",

This comment has been minimized.

Copy link
@draffensperger

draffensperger May 17, 2019

Contributor

Should this be opentelemetry-base similar to the @opencensus/opencensus-base for OpenCensus?

This comment has been minimized.

Copy link
@mayurkale22

mayurkale22 May 17, 2019

Author Contributor

Done. Thanks

@mayurkale22

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

@flands Can we merge this?

@mayurkale22

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

@rochdev Could you please review this?

@rochdev

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

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": {

This comment has been minimized.

Copy link
@rochdev

rochdev Jun 3, 2019

Member

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.

This comment has been minimized.

Copy link
@mayurkale22

mayurkale22 Jun 6, 2019

Author Contributor

Sure, done. "tdd": "yarn run test -- --watch-extensions ts --watch"

"license": "Apache-2.0",
"devDependencies": {
"lerna": "^3.13.4",
"typescript": "^3.4.5"

This comment has been minimized.

Copy link
@rochdev

rochdev Jun 3, 2019

Member

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?

This comment has been minimized.

Copy link
@rochdev

rochdev Jun 5, 2019

Member

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.

"types": "build/src/index.d.ts",
"scripts": {
"fix": "lerna run fix",
"postinstall": "npm run bootstrap",

This comment has been minimized.

Copy link
@rochdev

rochdev Jun 3, 2019

Member

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.

This comment has been minimized.

Copy link
@mayurkale22

mayurkale22 Jun 6, 2019

Author Contributor

done

@rochdev
Copy link
Member

left a comment

LGTM. Left a few comments about a few things I think are worth discussing, but the structure itself looks good 👍

@mayurkale22

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

@rochdev Added tdd script in watch mode, removed lockfiles (as discussed in the meeting) and used yarn wherever needed. PTAL

@rochdev

rochdev approved these changes Jun 6, 2019

Copy link
Member

left a comment

LGTM. The TS compilation rules may need to be adjusted, but this can happen according to the TypeScript Guideline discussion after this is merged.

@vmarchaud
Copy link
Member

left a comment

LGTM, just a quick remark on the ts config

"allowUnusedLabels": false,
"declaration": true,
"forceConsistentCasingInFileNames": true,
"lib": ["es2016"],

This comment has been minimized.

Copy link
@vmarchaud

vmarchaud Jun 7, 2019

Member

i don't think we need to bundle the es6 since we target es6,everything should be available at runtime

This comment has been minimized.

Copy link
@rochdev

rochdev Jun 7, 2019

Member

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)

This comment has been minimized.

Copy link
@mayurkale22

mayurkale22 Jun 7, 2019

Author Contributor

Done.

mayurkale22 added some commits May 15, 2019

@mayurkale22 mayurkale22 force-pushed the mayurkale22:initial_skeleton branch from d95fa67 to 539962a Jun 7, 2019

@mayurkale22 mayurkale22 merged commit efe9fab into open-telemetry:master Jun 10, 2019

1 check passed

cla/linuxfoundation mayurkale22 authorized
Details

@mayurkale22 mayurkale22 deleted the mayurkale22:initial_skeleton branch Jun 10, 2019

@mayurkale22 mayurkale22 referenced this pull request Jun 10, 2019

Closed

TypeScript Guideline #22

@rochdev rochdev referenced this pull request Jun 14, 2019

Closed

REQUEST: New membership for @rochdev #112

6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.