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

Rename HTTPText to TextMap Propagator. #793

Merged

Conversation

carlosalberto
Copy link
Contributor

Fixes #317

HttpText is renamed to Text for the Propagator component.

I remember we barely discussing the possibility of renaming it to AsciiText instead (or similar), but wanted to try out Text first, as it's simpler, but I'm open to feedback ;)

@carlosalberto carlosalberto requested review from a team as code owners August 12, 2020 19:27
@carlosalberto carlosalberto added area:api Cross language API specification issue spec:context Related to the specification/context directory spec:trace Related to the specification/trace directory labels Aug 12, 2020
@bogdandrutu bogdandrutu self-assigned this Aug 12, 2020
@Oberon00
Copy link
Member

I would name this "TextMapPropagator" or "TextHeaderMapPropagator" to emphasize that it operates on a map of key/value pairs as opposed to simply returning/accepting a string (which might be the interface for the binary propagator).

@carlosalberto
Copy link
Contributor Author

For the record, TextMap was the name we used back in OT ;)

@bogdandrutu
Copy link
Member

@carlosalberto @Oberon00 happy with TextMap.. as long as it is very clear what are the expectations for the keys and values in therms of encoding (ascii, utf-8, utf-16, url encoded, etc.)

@bogdandrutu
Copy link
Member

@carlosalberto ping

@carlosalberto
Copy link
Contributor Author

Renamed this to TextMap - should we list the keys/values encoding restrictions in a follow up?

@bogdandrutu
Copy link
Member

Would prefer now because before they were in the name otherwise this is a downgrade

Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

I'm fine with this name.

I don't think we need to put an encoding in the name. If we want to have an ASCII-only propagator, we can do this strongly typed, but we don't have to. We could have a getAsciiTextMapPropagator and getTextMapPropagator on the Propagators class/interface both returning an instance of TextMapPropagator.
If we implement this strongly typed, I suggest writing the spec in such a way that you can implement AsciiTextMapPropagator as an empty interface deriving from TextMapPropagator.

@carlosalberto
Copy link
Contributor Author

@bogdandrutu Added a note with a MUST - wondering about it after @Oberon00's comment. For reference, we had something like this as well in OpenTracing, although more of a suggestion (in order to increase compatibility).

To me, the most common case is still HTTP headers, so I'd vow to stick to ascii. For other encodings, the user should be able to (hopefully soon) use a Binary propagator instead ;)

Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
@carlosalberto carlosalberto changed the title Rename HTTPText to Text Propagator. Rename HTTPText to TextMap Propagator. Aug 20, 2020
@carlosalberto carlosalberto merged commit b409fb1 into open-telemetry:master Aug 21, 2020
TomRoSystems added a commit to TomRoSystems/opentelemetry-cpp that referenced this pull request Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue spec:context Related to the specification/context directory spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTPTextFormat naming (no HTTP)
5 participants