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

Assert correct elements are removed from hashes larger than limit. #1535

Conversation

ptolts
Copy link

@ptolts ptolts commented Oct 27, 2023

Adds missing test coverage for Tracestates larger than 32.

@linux-foundation-easycla
Copy link

CLA Not Signed

Copy link
Contributor

@fbogsany fbogsany left a comment

Choose a reason for hiding this comment

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

This is technically incorrect. From #1533 (comment)

Followup: the W3C trace-context spec states:

In a situation where tracestate needs to be truncated due to size limitations, the 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 that other truncation strategies like safe list entries, blocked list entries, or size-based truncation MAY be used, but are highly discouraged. Those strategies decrease the interoperability of various tracing vendors.

Our implementation is wrong and we should remove entries from the end rather than the start (and, of course, we should remove entries > 128 characters first).

@ptolts
Copy link
Author

ptolts commented Nov 1, 2023

I'll close, thanks Bogs.

@ptolts ptolts closed this Nov 1, 2023
@fbogsany
Copy link
Contributor

fbogsany commented Nov 2, 2023

I'll close, thanks Bogs.

😞 I was kinda hoping you'd fix the test and then we could fix the implementation 😄

@fbogsany fbogsany reopened this Nov 2, 2023
@fbogsany
Copy link
Contributor

fbogsany commented Nov 2, 2023

@ptolts could you please click the link in #1535 (comment) to authorize the CLA?

tracestate = OpenTelemetry::Trace::Tracestate.from_hash(input_hash)
output_hash = tracestate.to_h
_(output_hash.size).must_be :<=, 32
_(output_hash).must_equal(input_hash.drop(1).to_h)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_(output_hash).must_equal(input_hash.drop(1).to_h)
_(output_hash).must_equal(input_hash.first(32).to_h)

@kaylareopelle
Copy link
Contributor

Hi, @ptolts! Checking in on this PR. Would you be open to signing the CLA?

Copy link
Contributor

👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

@github-actions github-actions bot added the stale label Mar 22, 2024
@github-actions github-actions bot closed this Apr 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants