Skip to content

minor update: update method name of TraceContext#24826

Merged
yanavlasov merged 6 commits into
envoyproxy:mainfrom
wbpcode:dev-update-trace-context-name
Jan 25, 2023
Merged

minor update: update method name of TraceContext#24826
yanavlasov merged 6 commits into
envoyproxy:mainfrom
wbpcode:dev-update-trace-context-name

Conversation

@wbpcode
Copy link
Copy Markdown
Member

@wbpcode wbpcode commented Jan 9, 2023

Commit Message: minor update: update method name of TraceContext
Additional Description:

Update authority() of TraceContext to host() as the host is more widely used to represent k8s service name, dns domain, VIP, etc.

Risk Level: n/a.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
@KBaichoo
Copy link
Copy Markdown
Contributor

/assign-from @envoyproxy/envoy-maintainers

@repokitteh-read-only
Copy link
Copy Markdown

@envoyproxy/envoy-maintainers assignee is @kyessenov

🐱

Caused by: a #24826 (comment) was created by @KBaichoo.

see: more, trace.

@yanavlasov
Copy link
Copy Markdown
Contributor

I'm unsure this is the right naming. authority represents a well defined term in the HTTP standard and the header map returns the authority per the standard. host does not have this well defined meaning. I understand that host is used more widely in k8s but this code deals with HTTP, not k8s. As such authority is a more appropriate name here.

@yanavlasov
Copy link
Copy Markdown
Contributor

/wait-any

@kyessenov
Copy link
Copy Markdown
Contributor

kyessenov commented Jan 11, 2023

I tend to agree with Yan: authority is internally consistent with the rest of the Envoy codebase, for better or worse. There'll always be a nomenclature gap between the user and the machine APIs, that's why we have so many control planes that do the translation. User APIs are inherently ambiguous to permit options, while machine APIs are precise.

@kyessenov
Copy link
Copy Markdown
Contributor

/wait-any

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Jan 11, 2023

@yanavlasov yeah, authority is best for HTTP. But the TraceContext is designed as a protocol independent abstraction. It may be implemented by thrift, dubbo or some others third party protocol proxies.
So I want to use host here to represent the meaning of collection of instances (domain, VIP, registered service name, etc) of different protocols. 🤔

Cc @kyessenov

@kyessenov
Copy link
Copy Markdown
Contributor

I'll leave this one to Yan. I think it makes sense to generalize tracing beyond HTTP and follow https://opentelemetry.io/docs/reference/specification/trace/semantic_conventions/span-general/#nethostname.

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Jan 13, 2023

Friendly ping @yanavlasov

wbpcode added 5 commits January 19, 2023 00:31
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Jan 21, 2023

friendly ping @yanavlasov

@yanavlasov
Copy link
Copy Markdown
Contributor

Ok, makes sense. Thanks.

@yanavlasov yanavlasov merged commit 03192bf into envoyproxy:main Jan 25, 2023
@wbpcode wbpcode deleted the dev-update-trace-context-name branch January 25, 2023 01:57
VishalDamgude pushed a commit to freshworks-oss/envoy that referenced this pull request Feb 2, 2023
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
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.

4 participants