You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
following this PR, I noticed that some instrumentations has a types.ts file that contains both public and internal types mixed together.
I think this is not optimal, and want to suggest the following:
agree on a standart for how to structure the types in instrumentations (where to store internal / public types, how the files should be named, how to export them etc)
document it in the repo so new contributions / reviewers can use it as reference.
align all current instrumentations according to the standart.
If we get to an agreement, I can take the task to add documention markdown file to the repo and align all current instrumentations accordingly.
Public Types
Instrumentation config along with interfaces/types for hooks and filters (ignore matchers). example
Redecalring (copying) of types that we cannot import, because the package where they are declared includes implementation and is very large (aws-sdk).
A non backward compatible change in public types is breaking change and will be a problem once instrumentations are declared stable, so extra causion must be taken when adding types to this file. This is also why I think this change should be applied now while instrumentations are still experimental.
Internal Types
Meant to be consumed by instrumentation itself (instrumentation.ts, utils.ts etc) and should not be exported to instrumentation users. Changes can be made to these types which are guarenteed to not break users as they are not exported.
Types that extend some interface of the instrumented library itself. For example, adding new properties on Request for instrumentation purposes. exmaple
Declare types / interfaces used in the instrumentation logic itself. example
Options
Two Type Files (my personal preference)
one file for internal types and another file for public types.
Then, in index.ts we can export * from './types'; from the public types file, and in instrumentaiton.ts and other internal files we can import from the internal and public types files. This will help to prevent exporting internal types, and will introduce more structure into the codebase.
Suggestion for name: types.ts for public types. internal-types.ts for internal types. This is how winston instrumentation is built.
Single Types File
Put all types in a single types.ts file like we do now, but document that index.ts must export only public types and not *.
I am happy to get any comments and suggestions regarding this. We can possibly extend in the future to other aspects of instrumentation code structure and styling.
This only affects the JavaScript OpenTelemetry library
This may affect other libraries, but I would like to get opinions here first
The text was updated successfully, but these errors were encountered:
following this PR, I noticed that some instrumentations has a
types.ts
file that contains both public and internal types mixed together.I think this is not optimal, and want to suggest the following:
If we get to an agreement, I can take the task to add documention markdown file to the repo and align all current instrumentations accordingly.
Public Types
aws-sdk
).A non backward compatible change in public types is breaking change and will be a problem once instrumentations are declared stable, so extra causion must be taken when adding types to this file. This is also why I think this change should be applied now while instrumentations are still experimental.
Internal Types
Meant to be consumed by instrumentation itself (
instrumentation.ts
,utils.ts
etc) and should not be exported to instrumentation users. Changes can be made to these types which are guarenteed to not break users as they are not exported.Request
for instrumentation purposes. exmapleOptions
Two Type Files (my personal preference)
one file for internal types and another file for public types.
Then, in
index.ts
we canexport * from './types';
from the public types file, and ininstrumentaiton.ts
and other internal files we can import from the internal and public types files. This will help to prevent exporting internal types, and will introduce more structure into the codebase.Suggestion for name:
types.ts
for public types.internal-types.ts
for internal types. This is how winston instrumentation is built.Single Types File
Put all types in a single
types.ts
file like we do now, but document thatindex.ts
must export only public types and not*
.I am happy to get any comments and suggestions regarding this. We can possibly extend in the future to other aspects of instrumentation code structure and styling.
The text was updated successfully, but these errors were encountered: