Add validation for Trace ID by ymotongpoo · Pull Request #1992 · open-telemetry/opentelemetry-python

ymotongpoo · 2021-07-27T04:11:34Z

Description

This adds the validator in the constructor of SpanContext so that we can detect the invalid trace ID as early as possible.

Fixes #1991

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

TestSpanContext

I added the section in TestSpanContext in opentelemetry-api/tests/trace/test_span_context.py to validate Trace ID.

Does This PR Require a Contrib Repo Change?

Yes. - Link to PR:
No.

Checklist:

Followed the style guidelines of this project
Changelogs have been updated
Unit tests have been added
Documentation has been updated

owais · 2021-07-27T12:27:56Z

How does this work with pluggable ID generators? Will we be locking users out of using ID generators that do not strictly follow Otel spec? //cc @NathanielRN

ymotongpoo · 2021-07-27T14:29:09Z

When we just focus on the current implementations and other Python pluggable ID generators, this may introduce some breaking changes. But as I mentioned in #1991, some other languages such as Go and Java already introduced strict validation for the Trace ID format and the current Python implementation is causing incompatibility with other languages in that sense.

aabmass · 2021-07-27T14:57:09Z

opentelemetry-api/src/opentelemetry/trace/span.py

+            trace_id != INVALID_TRACE_ID
+            and span_id != INVALID_SPAN_ID


Should we add move these two checks into _validate_trace_id() so it's all together?

Instead, we should just put the code of _validate_trace_id here instead of having a single-use function. Keep in mind that the only thing that _validate_trace_id does is trace_id < 2 ** 128.

aabmass · 2021-07-27T15:03:30Z

opentelemetry-api/src/opentelemetry/trace/span.py

+    """
+    if not trace_id:
+        return False
+    if len(format(trace_id, "032x")) != _TRACE_ID_HEX_LENGTH:


Would this work and be a bit faster?

# constant somewhere _MAX_TRACE_ID = (1 << 128) - 1

Suggested change

if len(format(trace_id, "032x")) != _TRACE_ID_HEX_LENGTH:

if trace_id > _MAX_TRACE_ID:

Yes, I also prefer to make sure the trace id value is lesser than a certain value. Also, instead of (1 << 128) - 1 we can do 2 ** 128 - 1 which is subjectively easier to understand.

NathanielRN · 2021-07-27T18:28:39Z

@owais Thanks for the ping! Based on the PR right now, I think this should be fine, since it's only being strict about the length of the ID. It isn't giving restrictions about the bytes that actually make up the ID.

For example, in the AwsXRayIdGenerator we do this:

@staticmethod
def generate_trace_id() -> int:
    trace_time = int(time.time())
    trace_identifier = random.getrandbits(96)
    return (trace_time << 96) + trace_identifier

We remove some randomness from the ID in order to use some bits for the time stamp, but as far as OTel Python is concerned, this trace ID just as valid as an all random trace ID.

Then on the AWS backend, we can parse out this "OTel" ID as a "AWS ID" having expected the user used AwsXRayIdGenerator.

I think pluggable ID generators should follow the restrictions on ID length (so that we can count on this in the rest of OTel), but how systems encode/decode those bits should not be restricted so that they can add information they want.

NathanielRN · 2021-07-27T18:34:09Z

opentelemetry-api/src/opentelemetry/trace/span.py

+    if not trace_id:
+        return False
+    if len(format(trace_id, "032x")) != _TRACE_ID_HEX_LENGTH:
+        return False
+
+    return True


Building off of what @aabmass said. I'm okay with constant/no constant since it's only used once but having a _MAX_TRACE_ID_LENGTH is probably easier to read!

Suggested change

if not trace_id:

return False

if len(format(trace_id, "032x")) != _TRACE_ID_HEX_LENGTH:

return False

return True

return trace_id and trace_id < (1 << 128) - 1 and trace_id != INVALID_TRACE_ID

owais · 2021-07-27T18:51:46Z

As far as I could tell, Go only validates the ID during propagated context injection/extraction and I'm sure Python does it too already. Do we need this additional check?

ocelotl

Please avoid single use functions and single use constants. Remember that the validation of trace_id is just making sure it is not greater than a certain specific value.

opentelemetry-api/src/opentelemetry/trace/span.py

ocelotl · 2021-07-28T15:38:06Z

opentelemetry-api/src/opentelemetry/trace/span.py

+            trace_id != INVALID_TRACE_ID
+            and span_id != INVALID_SPAN_ID


Instead, we should just put the code of _validate_trace_id here instead of having a single-use function. Keep in mind that the only thing that _validate_trace_id does is trace_id < 2 ** 128.

ymotongpoo · 2021-07-29T02:29:43Z

Thank you for reviewing. Now I changed the code accordingly.

lzchen · 2021-07-30T15:55:36Z

@owais

As far as I could tell, Go only validates the ID during propagated context injection/extraction and I'm sure Python does it too already. Do we need this additional check?

I believe we only convert it to a base 16 int but not actually check the length.

aabmass · 2021-07-30T17:23:08Z

ID generators should follow the restrictions on ID length

As far as I could tell, Go only validates the ID during propagated context injection/extraction and I'm sure Python does it too already. Do we need this additional check?

I think this should be a requirement as it is in the OTel spec, OTLP proto semantics, and the W3C trace context. Whether or not it was wise to lock down the spec, idk.

@ymotongpoo pointed out that Go API uses a fixed 16 byte array here. @owais did you understand something different from the Go code?

aabmass · 2021-07-30T17:26:50Z

I believe we only convert it to a base 16 int but not actually check the length.

@lzchen we check it on extract() in the regex only, not inject:

opentelemetry-python/opentelemetry-api/src/opentelemetry/trace/propagation/tracecontext.py

Line 31 in 13f09db

"^[ \t]*([0-9a-f]{2})-([0-9a-f]{32})-([0-9a-f]{16})-([0-9a-f]{2})"

aabmass · 2021-07-30T17:38:18Z

opentelemetry-api/src/opentelemetry/trace/span.py

+        is_valid = (
+            trace_id != INVALID_TRACE_ID
+            and span_id != INVALID_SPAN_ID
+            and trace_id < 2 ** 128 - 1


@ocelotl nit regarding the single use constant, I think it's worth having the constant as it's easier to understand what the magic number means and for the speedup of not calculating the value every time in this hot code path.

(I was curious so checked and CPython is not smart enough to optimize this into a constant on its own (funny enough, it does do it for the bit shifting approach)):

In [2]: def f(trace_id): ...: return trace_id < 2 ** 128 - 1 ...: In [3]: dis(f) 2 0 LOAD_FAST 0 (trace_id) 2 LOAD_CONST 1 (2) 4 LOAD_CONST 2 (128) 6 BINARY_POWER 8 LOAD_CONST 3 (1) 10 BINARY_SUBTRACT 12 COMPARE_OP 0 (<) 14 RETURN_VALUE

Ok, that's a good point. In that case, the constant should be added as a private attribute of the class to keep it as close as where it is being used.

Not sure if we have specs somewhere, but my 2 cents is that I think (1 << 128) - 1 is easier to read.

I can go either way on the constant though!

ok I put the private const variable for readabiliy. As per bit shift vs multiplication, I leave the decision.

linux-foundation-easycla · 2021-08-02T00:07:28Z

The committers are authorized under a signed CLA.

✅ Yoshi Yamaguchi (cab7290, 9dedda0, aa86fcd, 8ec5d80, 54ddfb5, b41b2d2, 53e2cf1)
✅ Diego Hurtado (75b5f4a, 6b6674f)
✅ Leighton Chen (9b086d8)

ymotongpoo · 2021-08-02T00:31:48Z

I'm not sure what to do for EasyCLA. Because the merge from main was introduced here twice (75b5f4a and 9b086d8) after my last commit, I rebased and pushed b41b2d2. I don't know what to do to resolve this.

ocelotl · 2021-08-02T16:09:46Z

I'm not sure what to do for EasyCLA. Because the merge from main was introduced here twice (75b5f4a and 9b086d8) after my last commit, I rebased and pushed b41b2d2. I don't know what to do to resolve this.

Please try again, @ymotongpoo

ymotongpoo · 2021-08-02T16:32:34Z

@ocelotl Thanks! Now EasyCLA is fine.

ymotongpoo added 2 commits July 27, 2021 13:00

Add validation for Trace ID

cab7290

nit: Formatted to follow style guide

9dedda0

ymotongpoo requested review from a team, ocelotl and srikanthccv and removed request for a team July 27, 2021 04:11

ymotongpoo added 2 commits July 27, 2021 23:56

Add comments in Changelog

aa86fcd

fix typo

8ec5d80

aabmass reviewed Jul 27, 2021

View reviewed changes

NathanielRN reviewed Jul 27, 2021

View reviewed changes

ocelotl suggested changes Jul 28, 2021

View reviewed changes

Change to simplify the Trace ID validation

54ddfb5

ocelotl approved these changes Jul 29, 2021

View reviewed changes

Merge branch 'main' into b-1991

75b5f4a

Merge branch 'main' into b-1991

9b086d8

aabmass approved these changes Jul 30, 2021

View reviewed changes

Add private const variable for Trace ID hex length

b41b2d2

ymotongpoo and others added 2 commits August 2, 2021 09:37

nit: lint

53e2cf1

Merge branch 'main' into b-1991

6b6674f

lzchen merged commit 65670cf into open-telemetry:main Aug 2, 2021

	if len(format(trace_id, "032x")) != _TRACE_ID_HEX_LENGTH:
	if trace_id > _MAX_TRACE_ID:

Conversation

ymotongpoo commented Jul 27, 2021

Description

Type of change

How Has This Been Tested?

Does This PR Require a Contrib Repo Change?

Checklist:

Uh oh!

owais commented Jul 27, 2021

Uh oh!

ymotongpoo commented Jul 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NathanielRN commented Jul 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owais commented Jul 27, 2021

Uh oh!

ocelotl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ymotongpoo commented Jul 29, 2021

Uh oh!

lzchen commented Jul 30, 2021

Uh oh!

aabmass commented Jul 30, 2021

Uh oh!

aabmass commented Jul 30, 2021

Uh oh!

aabmass Jul 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linux-foundation-easycla bot commented Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ymotongpoo commented Aug 2, 2021

Uh oh!

ocelotl commented Aug 2, 2021

Uh oh!

ymotongpoo commented Aug 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

aabmass Jul 30, 2021 •

edited

Loading

linux-foundation-easycla bot commented Aug 2, 2021 •

edited

Loading