✨Add initial span interface #31

mayurkale22 · 2019-06-13T22:18:51Z

Completes #29

This is based on the specs: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/tracing-api.md#span

This PR contains following interfaces:

Span
SpanContext
Status
Link
Attributes
TraceOptions
TraceState
SpanKind

mayurkale22 · 2019-06-13T22:26:48Z

@rochdev @vmarchaud @hekike @bg451 Please review. Also, please file an issue here so that I can add you as a reviewer.

hekike · 2019-06-14T03:15:46Z

packages/opentelemetry-types/src/trace/span.ts

+   */
+  addEvent(
+    name: string,
+    attributes?: { [key: string]: string | number | boolean }


Is Object not supported here intentionally?
spec: values may be string, booleans or numeric type.
maybe would be a useful addition like we did for OpenCensus's setAttribute and automatically JSON.stringify() objects.
I assume setAttribute will do the same as spec is must be either a string, a boolean value, or a numeric type.

Added the object type.

hekike · 2019-06-14T03:16:17Z

packages/opentelemetry-types/src/trace/span.ts

+   */
+  addLink(
+    spanContext: SpanContext,
+    attributes?: { [key: string]: string | number | boolean }


vmarchaud · 2019-06-14T07:15:44Z

@mayurkale22 Which sponsor should we put on the issue ? I guess the first one is you but for the second one ?

danielkhan · 2019-06-14T07:33:42Z

packages/opentelemetry-types/src/trace/span.ts

+   * @param value the value for this attribute. If the value is a typeof object
+   *     it has to be JSON.stringify-able otherwise it will raise an exception.
+   */
+  setAttribute(key: string, value: string | number | boolean | object): void;


If those setters would return the Span itself (this), we would provide fluid interfaces, like span.setAttibute().addEvent().

I think value should be of type any similar to OpenTracing. The idea is that vendors can accept different values in different ways, so this should not be restricted.

There should also be setAttributes and setEvents to make it easier to add in bulk.

If those setters would return the Span itself (this), we would provide fluid interfaces, like span.setAttibute().addEvent().

Sure, done.

There should also be setAttributes and setEvents to make it easier to add in bulk.

Feel free to open an issue to discuss and if goes well, will include in the next iteration of the API.

rochdev · 2019-06-14T15:32:50Z

packages/opentelemetry-types/src/trace/span.ts

+   * Sets an attribute to the span.
+   *
+   * @param key the key for this attribute.
+   * @param value the value for this attribute. If the value is a typeof object


This would be problematic for Datadog since we currently accept values that are not JSON serializable and we have a custom serializer that knows how to handle them.

rochdev · 2019-06-14T15:33:13Z

packages/opentelemetry-types/src/trace/span.ts

+   */
+  addEvent(
+    name: string,
+    attributes?: { [key: string]: string | number | boolean | object }


Similar to the above, attributes values should be of type any.

I am fine with changing it to any type, but will lose type-safety and to mitigate this I have to add TSLint rule flag (tslint:disable-next-line:no-any). WDYT?

Since there is a limited number of types in JS it would be possible to simply specify all of them, but I think any is clearer that the intent is to accept anything. I'm personally fine with a TSLint comment. If it happens to much we could simply remove the rule. Code reviews will ensure that there is no abuse.

Could we use unknown instead of any? That allows the same semantics (the passed in value can be any type), but it requires type guards or a cast to actually do something with the type and so is safer from a type checking perspective. As a plus it doesn't require the lint comment. (Most times where you say any you can usually say unknown and get the same effect)

Would it make more sense then to include every possible JavaScript types? There are not that many and it would remove the need for casting for common types.

Could we use unknown instead of any?

For now, I have changed to unknown . If needed, will change to either any or every possible JavaScript types.

My understanding is that unknown is basically equivalent (maybe exactly equivalent?) to the union type of every possible type. You can use typeof and instanceof that would do runtime checks but are also used by the TS compiler to infer a more narrow type at compile time for code in code blocks guarded by those checks. Casts would only be needed if you somehow know what type it is and force it to that, which perhaps something vendor-specific might do.

See the function f20(x: unknown) example in the in the TS 3.0 release notes for an example of how typeof and instanceof can be used for type narrowing by the compiler (and of course checking of the value's actual JS runtime type so it can be correctly serialized).

rochdev · 2019-06-14T15:34:39Z

packages/opentelemetry-types/src/trace/span.ts

+   * Adds a link to the Span.
+   *
+   * @param spanContext the context of the linked span.
+   * @param [attributes] the attributes that will be added; these are


It's not clear what these attributes are used for.

I think attributes are the collection of key:value pairs associated with the link to conserve meaningful information related to link.

What I meant is that this should be better explained in the TypeDoc. In general I feel the documentation of the file doesn't explain much of what the different methods do. Of course, documentation can be improved in later iterations.

rochdev · 2019-06-14T15:36:00Z

packages/opentelemetry-types/src/trace/span.ts

+   *     associated with this link.
+   */
+  addLink(
+    spanContext: SpanContext,


This should accept a Span as well.

How is the type of link determined?

How is the type of link determined?

Good Point! AFAIK will use attributes to highlight the link type and will handle in sdk implementation. @songy23 and @rghetia to add more on this.

This should accept a Span as well.

Can you please mention the use case with span ?

Can you please mention the use case with span ?

In general, I think everywhere a SpanContext is accepted should also accept a Span. This avoids the caller having to always call span.context() if they are referencing a span. I don't have a specific use case but I've had a lot of cases where I was happy that OpenTracing did that for me. This is especially true if the context propagator returns the active span.

rochdev · 2019-06-14T15:36:44Z

packages/opentelemetry-types/src/trace/span.ts

+   *
+   * @param name the Span name.
+   */
+  updateName(name: string): void;


I would use setName to be consistent with setAttribute.

SGTM open-telemetry/opentelemetry-specification#119.

The decision is to go with updateName for this feature, so terminology will be aligned across languages. I will add TODO with issue number to consider this later.

rochdev · 2019-06-14T15:37:31Z

packages/opentelemetry-types/src/trace/span.ts

+   * Call to End of a Span MUST not have any effects on child spans. Those may
+   * still be running and can be ended later.
+   */
+  end(): void;


This should accept a timestamp since it's the only way to support some frameworks (i.e. Apollo Tracing).

Makes sense, done.

Shouldn't we change the spec for this?
https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/tracing-api.md#end

There MUST be no parameter.

I would say yes as the same issue applies to every language. The same will be true also for the ability to set the start time.

open-telemetry/opentelemetry-specification#120

rochdev · 2019-06-14T15:38:39Z

packages/opentelemetry-types/src/trace/span.ts

+   * @returns true if this Span is active and recording information like events
+   * with the AddEvent operation and attributes using setAttributes.
+   */
+  isRecordingEvents(): boolean;


From the description it sounds like the fact that it's recording events is only part of the state. Should this be called isActive instead or something generic?

This flag is used to indicate whether events should be recorded or not. If it's set, Events and Attributes, etc. will be recorded. Otherwise, these events will be dropped. The only way to set this flag when we create the span. If not set, the implementation will provide a default.

rochdev · 2019-06-14T15:40:59Z

packages/opentelemetry-types/src/trace/span_context.ts

+  /** The ID of the Span. */
+  spanId: string;
+  /** Trace options to propagate. */
+  traceOptions?: number;


It's unclear how this is supposed to be used. What does this number represent?

Addressed in 52e53ab, PTAL

Would it make more sense to have different fields instead of a single bit field? While it requires a little bit more memory, it makes such an interface a lot easier to understand and use.

rochdev · 2019-06-14T15:41:44Z

packages/opentelemetry-types/src/trace/span_context.ts

+  /** Trace options to propagate. */
+  traceOptions?: number;
+  /** Tracing-system-specific info to propagate. */
+  traceState?: string;


It's unclear what should be the format of this string.

Addressed in 52e53ab, PTAL

This would be easier to interact with by using an actual object than can then be serialized to a string by some formatter/encoder. Of course this could be done with a getter and a corresponding _traceState private property to hold the object, but then every vendor will have to replicate this logic.

Makes sense, added TraceState and TraceOptions classes with TODO comment.

rochdev · 2019-06-14T15:44:19Z

packages/opentelemetry-types/src/trace/status.ts

+}
+
+/** An enumeration of canonical status codes. */
+export enum CanonicalCode {


Does it make sense to have a fixed enum for this? What if the reason falls outside of the range of values?

Also, should there be a code for exceptions, with the error information attached?

I think will revise after open-telemetry/opentelemetry-specification#59. Currently it is copied from OpenCensus project.

draffensperger · 2019-06-15T03:04:47Z

packages/opentelemetry-types/src/trace/span.ts

+   * Do not return `this`. The Span generally should not be used after it
+   * is ended so chaining is not desired in this context.
+   *
+   * @param [endTime] the timestamp to set as Span's end time. If not provided,


Could you specify how the timestamp is formatted? Are we going to use epoch millis?

If you don't mind, I will update once we have some conclusion here #19

Sounds good. Could you add a TODO comment that points to that issue though?

draffensperger · 2019-06-15T03:05:19Z

packages/opentelemetry-types/src/trace/span_context.ts

+ * propagated along side of a distributed context.
+ */
+export interface SpanContext {
+  /** The ID of the trace that this span belongs to. */


Should this specify the format? Hex-encoded 128-bit number? Same for spanId.

Done 424c9fd

mayurkale22 · 2019-06-18T18:10:41Z

@hekike @rochdev @danielkhan @draffensperger I tried to address all the comments, please take a look again. I think this is just first iteration will update/add moving forward.

draffensperger

Gave a few more comments but LGTM overall.

draffensperger · 2019-06-18T23:12:40Z

packages/opentelemetry-types/src/trace/span.ts

+   * Do not return `this`. The Span generally should not be used after it
+   * is ended so chaining is not desired in this context.
+   *
+   * @param [endTime] the timestamp to set as Span's end time. If not provided,


Sounds good. Could you add a TODO comment that points to that issue though?

draffensperger · 2019-06-18T23:14:16Z

packages/opentelemetry-types/src/trace/span_context.ts

+   * sampled or not.
+   * SAMPLED_VALUE = 0x1 and NOT_SAMPLED_VALUE = 0x0;
+   */
+  traceOptions?: TraceOptions;


Could this just be a number instead of a TraceOptions structure? Seems like the comment implies that.

draffensperger · 2019-06-18T23:16:00Z

packages/opentelemetry-types/src/trace/span_context.ts

+   *     Multiple tracing systems (with different formatting):
+   *         tracestate: rojo=00f067aa0ba902b7,congo=t61rcWkgMzE
+   */
+  traceState?: TraceState;


Seems that the comment about is describing this as a string, but the type seems to imply something more complex - is this meant to become the parsed trace state?

I'm OK with leaving this as-is though since the structure is marked as TODO anyway.

draffensperger · 2019-06-18T23:16:51Z

packages/opentelemetry-types/src/trace/traceoptions.ts

+ * limitations under the License.
+ */
+
+export class TraceOptions {


Should the file name be something like trace_options.ts to match this?

Also, should this be an interface instead?

draffensperger · 2019-06-18T23:17:36Z

packages/opentelemetry-types/src/trace/tracestate.ts

+ * limitations under the License.
+ */
+
+export class TraceState {


Similar comments here: should the file be trace_state.ts, and should this be an interface? (I realize this is TODO, but wanted to comment anyway on this since this is the types package)

mayurkale22 · 2019-06-19T17:35:18Z

I think I addressed all of the comments by updating the PR, adding TODOs, or opening issues. Merging this PR now. Thanks for the reviews @danielkhan @rochdev @hekike @draffensperger

Add initial span interface

dff68bb

mayurkale22 added Discussion Issue or PR that needs/is extended discussion. API labels Jun 13, 2019

mayurkale22 requested review from danielkhan, iredelmeier, justindsmith and yurishkuro as code owners June 13, 2019 22:18

hekike mentioned this pull request Jun 14, 2019

REQUEST: New membership for @hekike open-telemetry/community#110

Closed

6 tasks

hekike reviewed Jun 14, 2019

View reviewed changes

Add object type attribute

2311095

danielkhan reviewed Jun 14, 2019

View reviewed changes

rochdev reviewed Jun 14, 2019

View reviewed changes

yurishkuro removed their request for review June 14, 2019 15:41

rochdev reviewed Jun 14, 2019

View reviewed changes

mayurkale22 mentioned this pull request Jun 14, 2019

API: Span - use SetName instead of UpdateName open-telemetry/opentelemetry-specification#119

Closed

return this and add optional endTime

fafce9f

mayurkale22 mentioned this pull request Jun 14, 2019

Add optional param during span end operation open-telemetry/opentelemetry-specification#120

Closed

mayurkale22 added 2 commits June 14, 2019 14:19

Use any and add TODO

db13c94

Update TraceState with examples and add TODO

52e53ab

draffensperger reviewed Jun 15, 2019

View reviewed changes

specify traceId and spanId format

424c9fd

rochdev mentioned this pull request Jun 17, 2019

API: Describe Status class open-telemetry/opentelemetry-specification#59

Closed

mayurkale22 added 2 commits June 17, 2019 10:22

use unknown instead of any

d969247

Add TraceOptions and TraceState empty class

01d1340

mayurkale22 force-pushed the span_api branch from 48d1d3b to 01d1340 Compare June 18, 2019 00:10

mayurkale22 added 2 commits June 17, 2019 22:42

Add Link and Attributes interfaces

53b8093

update index.ts

595465c

rochdev approved these changes Jun 18, 2019

View reviewed changes

Change Link:attributes as optional property

f1d2b7e

draffensperger approved these changes Jun 18, 2019

View reviewed changes

Fix review comments

5c92e51

hekike approved these changes Jun 19, 2019

View reviewed changes

Merge branch 'master' into span_api

f4803cb

mayurkale22 merged commit 802182e into open-telemetry:master Jun 19, 2019

mayurkale22 deleted the span_api branch June 19, 2019 17:37

mayurkale22 mentioned this pull request Jun 19, 2019

Add Span API #29

Closed

mayurkale22 added the Agreed label Jun 24, 2019

This was referenced Jun 24, 2019

Add TraceState #57

Merged

Allow span type along with SpanContext everywhere #58

Closed

✨Add initial span interface #31

✨Add initial span interface #31

Conversation

mayurkale22 commented Jun 13, 2019 • edited Loading

mayurkale22 commented Jun 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmarchaud commented Jun 14, 2019

danielkhan Jun 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rochdev Jun 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayurkale22 commented Jun 18, 2019

draffensperger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayurkale22 commented Jun 19, 2019

mayurkale22 commented Jun 13, 2019 •

edited

Loading

danielkhan Jun 14, 2019 •

edited

Loading

rochdev Jun 14, 2019 •

edited

Loading