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

[DRAFT] Update name and interface for textmapgetter/textmapsetter #45

Closed
wants to merge 5 commits into from

Conversation

plantfansam
Copy link
Contributor

@plantfansam plantfansam commented Oct 27, 2022

When working on #44 (see draft), I learned that ngx.req.set_header does not set response headers. There is also a special method for getting response headers (ngx.resp.get_headers()). To avoid confusion, I renamed text_map_getter and text_map_setter to request_header_getter and request_header_setter.

To maintain symmetry between request_header_* and response_header_*, I also updated the propagators to assume that ngx was passed to them (instead of ngx.req). If we do not change this assumption, then the trace context propagator and baggage propagator would accept ngx.req as a carrier, while the traceresponse propagator would accept ngx.

This would be a breaking change. It's OK if you'd rather not change the existing propagators @yangxikun, just let me know.

@plantfansam plantfansam changed the title Update name and interface for textmapgetter/textmapsetter [DRAFT] Update name and interface for textmapgetter/textmapsetter Oct 27, 2022
@plantfansam plantfansam marked this pull request as draft October 27, 2022 22:01
@yangxikun
Copy link
Owner

Seems we need a new "trace context propagator" to extract traceresponse from upstream response header and inject traceresponse to downstream response. So maybe we can keep the trace context propagator and baggage propagator accept ngx.req as a carrier.

Sorry for the quickly say "sure" on #44. After checking opentelemetry specification, I found there was not spec about traceresponse. And traceresponse still in w3c trace context draft, maybe we can wait for the opentelemetry specification to include traceresponse.

Does your service need this feature now?

@plantfansam
Copy link
Contributor Author

Sorry for the quickly say "sure" on #44. After checking opentelemetry specification, I found there was not spec about traceresponse. And traceresponse still in w3c trace context draft, maybe we can wait for the opentelemetry specification to include traceresponse.

No problem! We do need this feature, but we can implement elsewhere if you do not want it in opentelemetry-lua yet!

@plantfansam
Copy link
Contributor Author

Seems we need a new "trace context propagator" to extract traceresponse from upstream response header and inject traceresponse to downstream response.

Yes, I have a draft of that here 😄 plantfansam#2. I think it makes sense to keep carrier as ngx.req for the TextMapPropagator.

@yangxikun yangxikun added the hold will merge when some one really needs label Nov 1, 2022
@plantfansam
Copy link
Contributor Author

Closing this until we actually want it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hold will merge when some one really needs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants