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

✨Add initial span interface #31

Merged
merged 13 commits into from
Jun 19, 2019
5 changes: 5 additions & 0 deletions packages/opentelemetry-types/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,8 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/

export * from './trace/span';
export * from './trace/span_context';
export * from './trace/span_kind';
export * from './trace/status';
106 changes: 106 additions & 0 deletions packages/opentelemetry-types/src/trace/span.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
/**
* Copyright 2019, OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import { SpanContext } from './span_context';
import { Status } from './status';

/**
* An interface that represents a span. A span represents a single operation
* within a trace. Examples of span might include remote procedure calls or a
* in-process function calls to sub-components. A Trace has a single, top-level
* "root" Span that in turn may have zero or more child Spans, which in turn
* may have children.
*/
export interface Span {
/**
* Returns the SpanContext object associated with this Span.
*
* @returns the SpanContext object associated with this Span.
*/
context(): SpanContext;

// /**
// * # TODO
// * Returns the Tracer object used to create this Span.
// * https://github.com/open-telemetry/opentelemetry-specification/issues/21
// */
// tracer(): Tracer;

/**
* 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* it has to be JSON.stringify-able otherwise it will raise an exception.
*/
setAttribute(key: string, value: string | number | boolean | object): void;
Copy link
Contributor

@danielkhan danielkhan Jun 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sure, done.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* Adds an event to the Span.
*
* @param name the name of the event.
* @param [attributes] the attributes that will be added; these are
* associated with this event.
*/
addEvent(
name: string,
attributes?: { [key: string]: string | number | boolean | object }
Copy link
Member

@rochdev rochdev Jun 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

): void;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should accept a Span as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is the type of link determined?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

attributes?: { [key: string]: string | number | boolean | object }
): void;

/**
* Sets a status to the span. If used, this will override the default Span
* status. Default is 'OK'.
*
* @param status the Status to set.
*/
setStatus(status: Status): void;

/**
* Updates the Span name.
*
* @param name the Span name.
*/
updateName(name: string): void;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use setName to be consistent with setAttribute.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* Marks the end of Span execution and sets the current time as the span's
* end time.
*
* 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


/**
* Returns the flag whether this span will be recorded.
*
* @returns true if this Span is active and recording information like events
* with the AddEvent operation and attributes using setAttributes.
*/
isRecordingEvents(): boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
30 changes: 30 additions & 0 deletions packages/opentelemetry-types/src/trace/span_context.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/**
* Copyright 2019, OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/**
* A SpanContext represents the portion of a Span which must be serialized and
* propagated along side of a distributed context.
*/
export interface SpanContext {
/** The ID of the trace that this span belongs to. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 424c9fd

traceId: string;
/** The ID of the Span. */
spanId: string;
/** Trace options to propagate. */
traceOptions?: number;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 52e53ab, PTAL

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/** Tracing-system-specific info to propagate. */
traceState?: string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 52e53ab, PTAL

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
50 changes: 50 additions & 0 deletions packages/opentelemetry-types/src/trace/span_kind.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/**
* Copyright 2019, OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/**
* Type of span. Can be used to specify additional relationships between spans
* in addition to a parent/child relationship.
*/
export enum SpanKind {
/** Default value. Indicates that the span is used internally. */
INTERNAL = 0,

/**
* Indicates that the span covers server-side handling of an RPC or other
* remote request.
*/
SERVER = 1,

/**
* Indicates that the span covers the client-side wrapper around an RPC or
* other remote request.
*/
CLIENT = 2,

/**
* Indicates that the span describes producer sending a message to a
* broker. Unlike client and server, there is no direct critical path latency
* relationship between producer and consumer spans.
*/
PRODUCER = 3,

/**
* Indicates that the span describes consumer receiving a message from a
* broker. Unlike client and server, there is no direct critical path latency
* relationship between producer and consumer spans.
*/
CONSUMER = 4,
}
161 changes: 161 additions & 0 deletions packages/opentelemetry-types/src/trace/status.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
/**
* Copyright 2019, OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/**
* The status of a Span by providing a standard CanonicalCode in conjunction
* with an optional descriptive message.
*/
export interface Status {
/** The canonical code of this message. */
code: CanonicalCode;
/** A developer-facing error message. */
message?: string;
}

/** An enumeration of canonical status codes. */
export enum CanonicalCode {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/**
* Not an error; returned on success
*/
OK = 0,
/**
* The operation was cancelled (typically by the caller).
*/
CANCELLED = 1,
/**
* Unknown error. An example of where this error may be returned is
* if a status value received from another address space belongs to
* an error-space that is not known in this address space. Also
* errors raised by APIs that do not return enough error information
* may be converted to this error.
*/
UNKNOWN = 2,
/**
* Client specified an invalid argument. Note that this differs
* from FAILED_PRECONDITION. INVALID_ARGUMENT indicates arguments
* that are problematic regardless of the state of the system
* (e.g., a malformed file name).
*/
INVALID_ARGUMENT = 3,
/**
* Deadline expired before operation could complete. For operations
* that change the state of the system, this error may be returned
* even if the operation has completed successfully. For example, a
* successful response from a server could have been delayed long
* enough for the deadline to expire.
*/
DEADLINE_EXCEEDED = 4,
/**
* Some requested entity (e.g., file or directory) was not found.
*/
NOT_FOUND = 5,
/**
* Some entity that we attempted to create (e.g., file or directory)
* already exists.
*/
ALREADY_EXISTS = 6,
/**
* The caller does not have permission to execute the specified
* operation. PERMISSION_DENIED must not be used for rejections
* caused by exhausting some resource (use RESOURCE_EXHAUSTED
* instead for those errors). PERMISSION_DENIED must not be
* used if the caller can not be identified (use UNAUTHENTICATED
* instead for those errors).
*/
PERMISSION_DENIED = 7,
/**
* Some resource has been exhausted, perhaps a per-user quota, or
* perhaps the entire file system is out of space.
*/
RESOURCE_EXHAUSTED = 8,
/**
* Operation was rejected because the system is not in a state
* required for the operation's execution. For example, directory
* to be deleted may be non-empty, an rmdir operation is applied to
* a non-directory, etc.
*
* A litmus test that may help a service implementor in deciding
* between FAILED_PRECONDITION, ABORTED, and UNAVAILABLE:
*
* - Use UNAVAILABLE if the client can retry just the failing call.
* - Use ABORTED if the client should retry at a higher-level
* (e.g., restarting a read-modify-write sequence).
* - Use FAILED_PRECONDITION if the client should not retry until
* the system state has been explicitly fixed. E.g., if an "rmdir"
* fails because the directory is non-empty, FAILED_PRECONDITION
* should be returned since the client should not retry unless
* they have first fixed up the directory by deleting files from it.
* - Use FAILED_PRECONDITION if the client performs conditional
* REST Get/Update/Delete on a resource and the resource on the
* server does not match the condition. E.g., conflicting
* read-modify-write on the same resource.
*/
FAILED_PRECONDITION = 9,
/**
* The operation was aborted, typically due to a concurrency issue
* like sequencer check failures, transaction aborts, etc.
*
* See litmus test above for deciding between FAILED_PRECONDITION,
* ABORTED, and UNAVAILABLE.
*/
ABORTED = 10,
/**
* Operation was attempted past the valid range. E.g., seeking or
* reading past end of file.
*
* Unlike INVALID_ARGUMENT, this error indicates a problem that may
* be fixed if the system state changes. For example, a 32-bit file
* system will generate INVALID_ARGUMENT if asked to read at an
* offset that is not in the range [0,2^32-1], but it will generate
* OUT_OF_RANGE if asked to read from an offset past the current
* file size.
*
* There is a fair bit of overlap between FAILED_PRECONDITION and
* OUT_OF_RANGE. We recommend using OUT_OF_RANGE (the more specific
* error) when it applies so that callers who are iterating through
* a space can easily look for an OUT_OF_RANGE error to detect when
* they are done.
*/
OUT_OF_RANGE = 11,
/**
* Operation is not implemented or not supported/enabled in this service.
*/
UNIMPLEMENTED = 12,
/**
* Internal errors. Means some invariants expected by underlying
* system has been broken. If you see one of these errors,
* something is very broken.
*/
INTERNAL = 13,
/**
* The service is currently unavailable. This is a most likely a
* transient condition and may be corrected by retrying with
* a backoff.
*
* See litmus test above for deciding between FAILED_PRECONDITION,
* ABORTED, and UNAVAILABLE.
*/
UNAVAILABLE = 14,
/**
* Unrecoverable data loss or corruption.
*/
DATA_LOSS = 15,
/**
* The request does not have valid authentication credentials for the
* operation.
*/
UNAUTHENTICATED = 16,
}