Skip to content
This repository was archived by the owner on Oct 3, 2023. It is now read-only.

Implement TraceContext header#44

Merged
chingor13 merged 9 commits intomasterfrom
trace_context
Oct 18, 2017
Merged

Implement TraceContext header#44
chingor13 merged 9 commits intomasterfrom
trace_context

Conversation

@chingor13
Copy link
Copy Markdown
Member

@chingor13 chingor13 commented Oct 17, 2017

Fixes #43

Had to switch the internal storage of the spanId and parentSpanId to be a hex string because some versions of PHP still run 32-bit (so an integer won't fit). This also affects the interface for the extension (which is still pre-alpha so no big deal).

@chingor13 chingor13 requested a review from tmatsuo October 17, 2017 23:51
@chingor13
Copy link
Copy Markdown
Member Author

Doh, looks like we can't be storing the span id as an int as we're testing against 32-bit versions of PHP as well and the span id is a 64-bit integer (8 bytes).

*/
class TraceContextFormatter implements FormatterInterface
{
const CONTEXT_HEADER_FORMAT = '/([0-9a-f]{2})-(.*)/';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it possible that the hex values uses uppercase letters?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's not excluded in the spec, so I'll make sure it works for both.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! LG, so merge once you've done this

* @param TraceContext $context
* @return string
*/
public function serialize(TraceContext $context)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How do you add another serializer when the version 1 is released?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Most likely, we will add an option in the constructor for which version to output.

*/
class TraceContextFormatter implements FormatterInterface
{
const CONTEXT_HEADER_FORMAT = '/([0-9a-f]{2})-(.*)/';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! LG, so merge once you've done this

@chingor13 chingor13 merged commit 55f0f8c into master Oct 18, 2017
@chingor13 chingor13 deleted the trace_context branch October 18, 2017 20:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants