docs: add conventions and developer guide (#92)

Author: Amir Blum

README.md

@@ -12,6 +12,16 @@
     </a>    
 </p>
 
+<p>
+  <strong>
+    <a href="doc/conventions.md">Conventions<a/>
+    &nbsp;&nbsp;&bull;&nbsp;&nbsp;
+    <a href="doc/development-guide.md">Development Guide<a/>
+  </strong>
+</p>
+
+---
+
 js extensions for the [open-telemetry](https://opentelemetry.io/) project, from [Aspecto](https://www.aspecto.io/) with :heart:
 
 **Compatible with [otel v0.18.0](https://github.com/open-telemetry/opentelemetry-js/releases/tag/v0.18.0)**
@@ -35,4 +45,4 @@ For documentation using with the old [plugin](https://github.com/open-telemetry/
 - Versions **0.3.x** of the instrumentations are compatible with otel version v0.18.0
 - Versions **0.2.x** of the instrumentations are compatible with otel version v0.17.0
 - Versions **0.1.x** of the instrumentations are compatible with otel version v0.16.0
-- Versions **0.0.x** of the instrumentations are compatible with otel version v0.15.0
+- Versions **0.0.x** of the instrumentations are compatible with otel version v0.15.0

doc/conventions.md

@@ -0,0 +1,94 @@
+# Instrumentation Libraries Conventions
+This document is an extension to the [open telemetry Trace Semantic Conventions](https://github.com/open-telemetry/opentelemetry-specification/tree/main/specification/trace/semantic_conventions) document. It records additional conventions which we found useful for the instrumentations in this repo.
+
+## Instrumentation Configuration
+
+### moduleVersionAttributeName
+```
+Type: string
+Required in each plugin: true
+Default: undefined
+```
+
+Instrumentation SHOULD support automatic collection of instrumented library version into a span attribute. The span attribute name is the value of the `moduleVersionAttributeName` option, and the span attribute value is the version of the instrumented library.
+
+If instrumentation library is patching multiple npm packages, each with a different version, then the library should pick the version of the most suited package and attach it to this attribute.
+
+### suppressInternalInstrumentation
+```
+Type: boolean
+Required in each plugin: false
+Default: false
+```
+
+Some instrumented libraries are using internally other packages which are instrumented as well. For example:
+- aws-sdk is using http for the internal RPC requests to various services.
+- elasticsearch client is communicating with the db via http.
+- mongoose (and other ORMs), are using database drivers to access the database.
+
+For some users, viewing the internal details of clients underlying protocol is important. Others only takes interest in the client instrumentation, and instrumenting the communication protocol just creates extra visual and performance load.
+
+If this attribute is set to `true`, then the instrumentation library should suppress the internal spans which are generated by calls to library functions.
+
+This is an optional config option, which should be used only in libraries which use an instrumented underlying protocol. Notice that suppressInstrumentation is not recommended when context needs to be propagated. So if propagation of context is used in some use case (supported by the receiving side), then this option should not be used.
+
+### ignoreOrphanedSpans
+```
+Type: boolean
+Required in each plugin: false
+Default: false
+```
+
+The `ignoreOrphanedSpans` is not well standardized across the opentelemetry js ecosystem. It is sometimes called by the names `requireParent` and `requireParentSpan` in contrib and core repo.
+
+In general, we can categorized traces into two categories:
+1. Traces that starts due to some external input to the application, usually a user-triggered event (incoming http request, message received on queue, etc).
+2. Traces that are triggered by some internal behavior:
+- Timers
+- Initialization code
+- Operation that have a trace context, but it was lost while running the code. This can happen when using [thenables](https://github.com/nodejs/node/issues/22360), event emitters, and other cases which break context management.
+
+Some users want to see all the operations in the application, not hiding anything. Those users will set `ignoreOrphanedSpans` to `false` (or leave it with the default value which is `false`).
+
+Other users may want to see only "external-triggered" traces, and reduce the noisy and performance impact by not instrumenting the internal operations. Those users will want to set the `ignoreOrphanedSpans` option to `true`.
+
+Ignoring spans can be done in many flavors, and the relevant considerations should be taken into account for each case, and documented for each library.
+- skip instrumentation - just call the original function and don't executed any instrumentation related code.
+- suppressInstrumentation - this method will also ignore downstream spans, which might not be desired.
+- NoRecordingSpan - create no recording span instead of a regular recording span. This method can influence downstream spans which are using parent based sampling.
+Unless there is a good reason not to, instrumentation libraries should ignore orphaned spans by skipping instrumentation (calling `original.apply(this, arguments)`).
+
+### Hooks
+```
+Type: function
+Required in each plugin: true
+Default: undefined
+```
+
+Each instrumentation library should expose hooks which can be configured to add custom logic and attributes to span on various lifecycle events.
+This will usually be `requestHook` and `responseHook`, but can be anything depending on the specific case.
+
+Each hook is a function that receives relevant parameters. Some common parameters are: 
+- The current span which is relevant to the hook
+- Original call parameters
+- Response that will be returned to user
+- Version of the instrumented library.
+
+The hooks can be used for any purpose, but some common use cases are:
+- Capturing additional operation data into custom span attributes (http headers, payloads, etc).
+- Registering to event emitters or patching objects for custom instrumentation extensions.
+- Setting global attributes for the component which are not part of the semantic conventions.
+
+Exceptions should not be thrown from hook functions, but instrumentation should protect itself for this case by catching hooks exceptions and write to diag logger. 
+
+## Logging
+Instrumentations should write diagnostic logs to `diag` api in `@opentelemetry/api`.
+Events that should be written are:
+- (info) Patching and unpatching of library functions
+- (error) Exception thrown from custom hook
+
+The log message should have the prefix `${component} instrumentation: ${message}`, for example: `mongoose instrumentation: patching mongoose Aggregate exec prototype`.
+
+## Instrumentation Class
+All the private functions in instrumentation class should have camelCase, with no leading underscore:
+(`private patchConnect(...)`).

doc/development-guide.md

@@ -0,0 +1,51 @@
+# Developer Guide
+This guide audience is developers writing a new instrumentation, maintaining existing instrumentation, or trying to understand the architecture and design decisions of instrumentation in this repo.
+
+It is meant to document various aspects of developing instrumentation library, based on the experiences we gained with the packages in this repo.
+
+## Unpatching
+TODO
+
+## Configuration
+Each instrumentation library should have configuration interface under `types.ts`. All the configuration parameters must be optional, and should have default value which is documented in the README.md of the package.
+
+Supplied configuration should be merged with default configuration object so that defaults are obvious:
+
+```js
+export const DEFAULT_CONFIG: FooInstrumentationConfig = {
+    barOptions: 'some default value for bar',
+};
+
+export class FooInstrumentation extends InstrumentationBase<typeof foo> {
+    protected _config: FooInstrumentationConfig;
+
+    constructor(config: FooInstrumentationConfig = {}) {
+        super('opentelemetry-instrumentation-amqplib', VERSION, Object.assign({}, DEFAULT_CONFIG, config));
+    }
+}
+```
+
+## Holding References to Objects
+TODO
+
+## Close Span on Exception
+Each span that is started should be ended. This is usually done after receiving the response for the operation.
+A common error is not ending the span in case an exception was thrown (skips the code that calls `span.end()`).
+
+You can use the `safeExecuteInTheMiddle` util from `@opentelemetry/instrumentation` for this task.
+
+## Using Mocks vs Docker in Tests
+TODO
+
+## Utils vs Instrumentation Class
+TODO
+
+## Test All Versions
+Each instrumentation must set package.json script called `test:ci` which runs [test-all-versions](https://www.npmjs.com/package/test-all-versions) on all the versions which are supported by the instrumentation.
+
+# Checklist before submitting a PR
+- [ ] Supported version range is specified
+- [ ] [test-all-versions](https://www.npmjs.com/package/test-all-versions) is configured to run on `test:ci` with all supported versions.
+- [ ] PR to add new instrumentation to [open telemetry registry](https://github.com/open-telemetry/opentelemetry.io)
+- [ ] Verify that instrumented library is a dev dependency
+- [ ] Verify you only import from instrumented library with type: `import type x from 'y'`. A valid otel setup can install the plugin but not the instrumented library, in which case importing directly will fail and crash the application.