Add TraceState #57

mayurkale22 · 2019-06-24T22:42:38Z

Specs: https://w3c.github.io/trace-context/#tracestate-field

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.

bg451 · 2019-06-25T00:52:23Z

Are there existing use cases where a vendor checks the tracestate size and truncates kv pairs until it's under the size limit? I don't think I've seen a vendor use tracestate/baggage that way yet, but might be worth creating an issue to add that functionality.

packages/opentelemetry-core/src/trace/TraceState.ts

mayurkale22 · 2019-06-25T03:16:23Z

Are there existing use cases where a vendor checks the tracestate size and truncates kv pairs until it's under the size limit? I don't think I've seen a vendor use tracestate/baggage that way yet, but might be worth creating an issue to add that functionality.

Good question, but I haven't heard about these use cases. I would suggest to open an issue in specification repo.

packages/opentelemetry-core/src/trace/TraceState.ts

rochdev · 2019-06-25T16:32:09Z

packages/opentelemetry-core/src/trace/TraceState.ts

+import * as types from '@opentelemetry/types';
+
+// TODO validate maximum number of items
+// const maximumTraceStateItems = 32;


Why would this be limited? What if a vendor needs more than the limit?

This limit is enforced by the specs, "There can be a maximum of 32 list-members in a list.".

I think it should be fine for now as 32 is somewhat high for this. It may be worth it however to start a discussion about the reasoning for the limit. Since there is already a maximum length for the whole state, it doesn't really make sense to add such an additional limit.

@rochdev one vendor can only have one key-value pair.
Here is an explanation of how entries are added and replaced: https://github.com/w3c/trace-context/blob/master/spec/20-http_header_format.md#relationship-between-the-headers

rochdev · 2019-06-25T16:33:46Z

packages/opentelemetry-core/src/trace/TraceState.ts

+
+const maximumTraceStateLength = 512;
+export interface InternalTraceState {
+  [key: string]: string;


Why is this limited to strings? We store other types in the trace state.

I think this is easier for serialization/deserialization and this is how other languages are keeping it. What are the types do you use/prefer in the trace state?

The problem is if other types of object are stored and manipulated during the lifetime of the local trace, then conversion will constantly be needed. This is generally a concern for anywhere that accepts only string values, but in this case it may be fine since the trace state properties are usually not altered after they are set.

Actually, if this is only used at propagation time to another service, my above concern doesn't exist. This is especially true if there is a single trace state by vendor.

packages/opentelemetry-core/src/trace/TraceState.ts

rochdev · 2019-06-25T16:40:22Z

packages/opentelemetry-core/src/trace/TraceState.ts

+      agg.push(`${key}=${state.get(key)}`);
+      return agg;
+    }, [])
+    .join(',');


This format is very difficult to work with, especially for trace states with nested objects. Why not simply rely on JSON?

Looks like this is enforced by W3C TraceContext. We should investigate how to properly support the , and = characters, and also nested objects.

Value is opaque string up to 256 characters printable ASCII [[!RFC0020]] characters (i.e., the range 0x20 to 0x7E) except comma , and =. Note that this also excludes tabs, newlines, carriage returns, etc.

What will go into value needs to be specified in OpenTelemetry. This is not a general purpose list for user-provided baggage.

rochdev · 2019-06-25T16:45:09Z

packages/opentelemetry-core/src/trace/TraceState.ts

+  [key: string]: string;
+}
+
+export class TraceState implements types.TraceState {


Where will this be stored? It also looks like this conflicts with DistributedContext as this is the equivalent of one of the class described in open-telemetry/opentelemetry-specification#140?

mayurkale22 · 2019-06-25T21:56:37Z

I think I addressed all of the comments by updating the PR and adding TODOs. PTAL

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

packages/opentelemetry-core/src/trace/TraceState.ts

draffensperger · 2019-06-26T19:48:10Z

packages/opentelemetry-core/src/trace/TraceState.ts

+    this.internalState.set(name, value);
+  }
+
+  serialize(): string {


We had talked today in the SIG about doing some kind of encoding/decoding of values. E.g. using URL encoding for the unsupported characters. Is there any kind of encoding format for this specified in the spec?

There isn't in the w3c spec nor the opentelem spec. I think as an instrumentation library we should only attempt to validate the user's input and provide a stern warning (but not exception) that the input is invalid. I wouldn't want to automatically encode the input when it goes over the wire in case they aren't using an opentelem library downstream. I feel like if the user wants to include non spec conforming data and have it accessible downstream, we can recommend using URL encoding.

This is pushing unnecessary complexity on the user. I understand if W3C don't want to enforce a specific format, but I think OpenTelemetry should standardize this.

+1 on OpenTelem standardizing on it. For what it's worth I'm not super stoked on using tracestate to propagate baggage to begin with, definitely interested in seeing how the new w3c correlation context turns out.

The intent of tracestate is to encode tracing system specific information and it's up to the tracing system how this is encoded. Some vendors use base64 to send some json, some simply have information like a tenant id in plain text.
At some point Opentelemetry needs to come up with what should be put in there and was there is quite some overlap between the w3c group and this project it will definitely be compliant.

The intent is NOT to make it available to the user to use it for baggage. As @bg451 said, this will be up to another spec.

With your above explanation that each vendor should have a single entry, it makes sense to not require a specific format. However, if there is a entry reserved for OpenTelemetry itself, then we should define the format of that entry.

packages/opentelemetry-core/src/trace/TraceState.ts

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

- Give more descriptive name than `s` to variable. - Private properties prefixed with an underscore for JavaScript users. - Private methods prefixed with an underscore for JavaScript users.

danielkhan · 2019-06-30T21:10:02Z

Are there existing use cases where a vendor checks the tracestate size and truncates kv pairs until it's under the size limit? I don't think I've seen a vendor use tracestate/baggage that way yet, but might be worth creating an issue to add that functionality.

@bg451 - the reason why this limit was added is to prevent users from abusing tracestate for baggage of arbitrary length. This is a requirement that came from cloud vendors.

This rule allows a cloud vendor to truncate tracestate back to a reasonable size if needed.

danielkhan

I'm sure that we will have to add a bit more work - especially for dealing with limits - but this is a good start.

* Add TraceState * Fix review comments * Minor Fix, add TODOs * Use Map to preserve ordering * Add underscore for private methods & rename variable - Give more descriptive name than `s` to variable. - Private properties prefixed with an underscore for JavaScript users. - Private methods prefixed with an underscore for JavaScript users. * Remove TEST_ONLY methods * Add comment about TraceState class * Add unset method

rochdev · 2019-07-02T13:38:43Z

This rule allows a cloud vendor to truncate tracestate back to a reasonable size if needed.

What happens if one vendor, or a combination of vendors, breaks the rule? Will this break the state for every vendor?

danielkhan · 2019-07-02T13:55:12Z

This rule allows a cloud vendor to truncate tracestate back to a reasonable size if needed.

What happens if one vendor, or a combination of vendors, breaks the rule? Will this break the state for every vendor?

Here is the spec:

In situation when tracestate needs to be truncated due to size limitations, platform of tracing vendor MUST truncate whole entries. Entries larger than 128 characters long SHOULD be removed first. Then entries SHOULD be removed starting from the end of tracestate. Note, other truncation strategies like safe list entries, blocked list entries or size-based truncation MAY be used, but highly discouraged. Those strategies will decrease interoperability of various tracing vendors.

So the rule describes a procedure of how to deal with entries that are too long.
If this rule wouldn't be in place, the state could break for other vendors if one violates the size limits and another system just starts throwing away entries until the size requirements are met again.

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

mayurkale22 added this to the API complete milestone Jun 24, 2019

mayurkale22 requested review from danielkhan, iredelmeier and justindsmith as code owners June 24, 2019 22:42

mayurkale22 requested a review from rochdev June 24, 2019 22:42

draffensperger reviewed Jun 25, 2019

View reviewed changes

packages/opentelemetry-core/src/trace/TraceState.ts Outdated Show resolved Hide resolved

draffensperger reviewed Jun 25, 2019

View reviewed changes

packages/opentelemetry-core/src/trace/TraceState.ts Outdated Show resolved Hide resolved

packages/opentelemetry-core/src/trace/TraceState.ts Outdated Show resolved Hide resolved

packages/opentelemetry-core/src/trace/TraceState.ts Outdated Show resolved Hide resolved

rochdev reviewed Jun 25, 2019

View reviewed changes

packages/opentelemetry-core/src/trace/TraceState.ts Outdated Show resolved Hide resolved

rochdev reviewed Jun 25, 2019

View reviewed changes

packages/opentelemetry-core/src/trace/TraceState.ts Outdated Show resolved Hide resolved

rochdev reviewed Jun 25, 2019

View reviewed changes

bg451 reviewed Jun 25, 2019

View reviewed changes

packages/opentelemetry-types/src/trace/trace_state.ts Outdated Show resolved Hide resolved

bg451 mentioned this pull request Jun 25, 2019

REQUEST: New membership for <your-GH-handle> open-telemetry/community#135

Closed

6 tasks

draffensperger reviewed Jun 26, 2019

View reviewed changes

rochdev reviewed Jun 26, 2019

View reviewed changes

packages/opentelemetry-core/src/trace/TraceState.ts Outdated Show resolved Hide resolved

rochdev reviewed Jun 26, 2019

View reviewed changes

packages/opentelemetry-core/src/trace/TraceState.ts Outdated Show resolved Hide resolved

rochdev reviewed Jun 26, 2019

View reviewed changes

packages/opentelemetry-core/src/trace/TraceState.ts Outdated Show resolved Hide resolved

draffensperger reviewed Jun 26, 2019

View reviewed changes

packages/opentelemetry-core/src/trace/TraceState.ts Outdated Show resolved Hide resolved

draffensperger reviewed Jun 26, 2019

View reviewed changes

packages/opentelemetry-core/src/trace/TraceState.ts Outdated Show resolved Hide resolved

rochdev reviewed Jun 27, 2019

View reviewed changes

packages/opentelemetry-types/src/trace/trace_state.ts Show resolved Hide resolved

mayurkale22 added 3 commits June 28, 2019 09:59

Add TraceState

50175e5

Fix review comments

59abd67

Minor Fix, add TODOs

88f47cf

mayurkale22 added 4 commits June 28, 2019 09:59

Use Map to preserve ordering

c681e66

Add underscore for private methods & rename variable

17a3c85

- Give more descriptive name than `s` to variable. - Private properties prefixed with an underscore for JavaScript users. - Private methods prefixed with an underscore for JavaScript users.

Remove TEST_ONLY methods

c321a59

Add comment about TraceState class

544f3cf

mayurkale22 force-pushed the trace_state branch from e21d549 to 544f3cf Compare June 28, 2019 17:35

Add unset method

7342a34

danielkhan approved these changes Jun 30, 2019

View reviewed changes

Merge branch 'master' into trace_state

58619d1

mayurkale22 merged commit 7b629d6 into open-telemetry:master Jul 1, 2019

mayurkale22 deleted the trace_state branch July 1, 2019 18:53

This was referenced Aug 6, 2019

Update JavaScript maintainers open-telemetry/community#155

Merged

Nominating @bg451 as JavaScript approver open-telemetry/community#157

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TraceState #57

Add TraceState #57

mayurkale22 commented Jun 24, 2019

bg451 commented Jun 25, 2019

mayurkale22 commented Jun 25, 2019

rochdev Jun 25, 2019

mayurkale22 Jun 25, 2019

rochdev Jun 26, 2019

danielkhan Jun 30, 2019

rochdev Jun 25, 2019

mayurkale22 Jun 25, 2019

rochdev Jun 26, 2019

rochdev Jul 2, 2019

rochdev Jun 25, 2019

rochdev Jun 25, 2019

danielkhan Jun 30, 2019 •

edited

Loading

rochdev Jun 25, 2019

mayurkale22 commented Jun 25, 2019

draffensperger Jun 26, 2019

bg451 Jun 26, 2019

rochdev Jun 27, 2019

bg451 Jun 27, 2019

danielkhan Jun 30, 2019

rochdev Jul 2, 2019

danielkhan commented Jun 30, 2019

danielkhan left a comment

rochdev commented Jul 2, 2019

danielkhan commented Jul 2, 2019

Add TraceState #57

Add TraceState #57

Conversation

mayurkale22 commented Jun 24, 2019

bg451 commented Jun 25, 2019

mayurkale22 commented Jun 25, 2019

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

danielkhan Jun 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayurkale22 commented Jun 25, 2019

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

danielkhan commented Jun 30, 2019

danielkhan left a comment

Choose a reason for hiding this comment

rochdev commented Jul 2, 2019

danielkhan commented Jul 2, 2019

danielkhan Jun 30, 2019 •

edited

Loading