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

Initial proposal #1

Merged
merged 2 commits into from
Oct 20, 2020
Merged

Initial proposal #1

merged 2 commits into from
Oct 20, 2020

Conversation

SergeyKanzhelev
Copy link
Member

This PR was moved from w3c/trace-context#217

CC: @lmolkova @clemensv @calohmn @AloisReitbauer @yurishkuro @grs

I'm moving comments from original PR as well

created, it’s no longer permissible to edit the bare message. So if it were
necessary to annotate the message inside the middleware as it transits, that
MUST happen in the “message-annotations” section, using the
Copy link
Member Author

Choose a reason for hiding this comment

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

Comment was resolved in original PR:

image

truncating or reusing some fields.

## Batching
Copy link
Member Author

Choose a reason for hiding this comment

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

Comment for original PR:

image

AMQP defines message as a payload with the additional annotations sections.
There are two annotation sections this specification refers -
"application-properties" and "message-annotations". See
Copy link
Member Author

Choose a reason for hiding this comment

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

Comment from original PR:

image
image

of AMQP specification.

Property name MUST be `traceparent` - all lowercase without delimiters.
Copy link
Member Author

Choose a reason for hiding this comment

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

from: @calohmn

Considering the case that the traceparent and tracestate fields are transported via message-annotations:

As for the base type of the message-annotations, the AMQP spec says (http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-messaging-v1.0-os.html#type-annotations):

The annotations type is a map where the keys are restricted to be of type symbol or of type ulong. All ulong keys, and all symbolic keys except those beginning with "x-" are reserved. Keys beginning with "x-opt-" MUST be ignored if not understood. On receiving an annotation key which is not understood, and which does not begin with "x-opt", the receiving AMQP container MUST detach the link with a not-implemented error.

Concerning the names of message-annotations:

A registry of defined annotations and their meanings is maintained [AMQPMESSANN].

The question here: Are the names of the traceparent and tracestate fields supposed to be somehow registered as reserved with OASIS. Otherwise it looks to me as if the prefix "x-opt-" should be used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also created this issue: #2

Choose a reason for hiding this comment

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

As I wrote in the issue, we can register this fields in AMQP TC

See definition of type map in section
[1.6.23](http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-types-v1.0-os.html#type-map)
of AMQP specification.
Copy link
Member Author

Choose a reason for hiding this comment

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

Comment from @calohmn

Considering the case that the traceparent and tracestate fields are transported via application-properties:

About application-properties, the AMQP spec says (http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-messaging-v1.0-os.html#type-application-properties):

The keys of this map are restricted to be of type string (which excludes the possibility of a null key) and the values are restricted to be of simple types only, that is, excluding map, list, and array types.

This looks incompatible with wanting to store a tracestate AMQP map in there.

Choose a reason for hiding this comment

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

HTTP headers are constrained to strings and you still manage to put a map there.

Copy link

Choose a reason for hiding this comment

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

I was referring to the AMQP map datatype being mentioned for the tracestate field value here.
As for using a string value instead, I guess it should be clarified then, how exactly the string-to-string map is to be encoded in that string value.

Copy link

Choose a reason for hiding this comment

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

it is impractical to ask everyone to parse tracestate - the majority of services will blindly propagate it:

  1. receive incoming http request with tracestate as string
  2. put tracestate into EventHub message. If it is a map - it will require parsing it in the first place which should not be necessary for arbitrary service

So my proposal: it should be either a string (exactly like http-header) or byte array to save n message size (and this spec should define how it is formatted).


Strings duplicating the size of a field, using list of binaries will require to
redefine the way the field serialized, parsed, and versioned. So re-using binary
protocol looks like a logical solution.
Copy link
Member Author

Choose a reason for hiding this comment

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

Comment from original PR:

image

such problems.

TODO: do we need to define a way to specify trace context of read operation? Is
Copy link

@clemensv clemensv Mar 25, 2019

Choose a reason for hiding this comment

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

I'm not sure what you're referring to here. If there's an error in delivering the message (delivery state "rejected"), that state includes an error object and that can carry an extra "info" map, which could conceivably carry trace objects.

Copy link
Contributor

@nikmd23 nikmd23 left a comment

Choose a reason for hiding this comment

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

Great start. Thanks for getting this going!

spec/20-AMQP_FORMAT.md Outdated Show resolved Hide resolved
Co-Authored-By: SergeyKanzhelev <[email protected]>
@SergeyKanzhelev
Copy link
Member Author

I'll merge this proposal and will file issues from the comments

@SergeyKanzhelev SergeyKanzhelev merged commit b1542f5 into master Oct 20, 2020
@SergeyKanzhelev SergeyKanzhelev deleted the sergkanz/originalProposal branch October 20, 2020 18:37
@SergeyKanzhelev SergeyKanzhelev restored the sergkanz/originalProposal branch October 20, 2020 18:39
@plehegar plehegar deleted the sergkanz/originalProposal branch May 18, 2021 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants