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

(Don't?) rename HTTPText to TextMap Propagator #331

Closed
fbogsany opened this issue Aug 24, 2020 · 2 comments · Fixed by #335
Closed

(Don't?) rename HTTPText to TextMap Propagator #331

fbogsany opened this issue Aug 24, 2020 · 2 comments · Fixed by #335

Comments

@fbogsany
Copy link
Contributor

open-telemetry/opentelemetry-specification#793 renamed the HttpTextPropagator to TextMapPropagator. We have a split in our implementation between http and text propagation

class Propagation
# Get or set the global http propagator. Use a CompositePropagator
# to propagate multiple formats.
attr_accessor :http
# Get or set the global text propagator. Use a CompositePropagator
# to propagate multiple formats.
attr_accessor :text

def configure_propagation
OpenTelemetry.propagation.http = create_propagator(@http_injectors || default_http_injectors,
@http_extractors || default_http_extractors)
OpenTelemetry.propagation.text = create_propagator(@text_injectors || default_text_injectors,
@text_extractors || default_text_extractors)
end

because we need to support Rack extractors for HTTP:

def default_http_extractors
[
OpenTelemetry::Trace::Propagation::TraceContext.rack_extractor,
OpenTelemetry::CorrelationContext::Propagation.rack_extractor
]
end

vs

def default_text_extractors
[
OpenTelemetry::Trace::Propagation::TraceContext.text_extractor,
OpenTelemetry::CorrelationContext::Propagation.text_extractor
]
end

I don't think we should change anything in our implementation, since the Ruby HTTP ecosystem really requires this distinction (almost?) everywhere. I'm just creating this issue to solicit and record agreement.

cc @mwear

@mwear
Copy link
Member

mwear commented Aug 24, 2020

I agree it's useful to have http and text versions of the propagators readily available due to Rack. I'm wondering if we could "meet in the middle" by calling our TextInjector / Extractor a TextMapInjector / Extractor. We could keep the http and text distinction, but the types would line up with the spec. I'm also fine with keeping things as-is.

@fbogsany
Copy link
Contributor Author

I'm wondering if we could "meet in the middle" by calling our TextInjector / Extractor a TextMapInjector / Extractor

👍 sounds good to me -- I'll PR that, perhaps tomorrow.

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 a pull request may close this issue.

2 participants